Fix nrf52840 serial mentioned #1021 by hathach · Pull Request #1036 · adafruit/circuitpython · GitHub
Skip to content

Fix nrf52840 serial mentioned #1021#1036

Merged
arturo182 merged 4 commits into
adafruit:masterfrom
hathach:fix_nrf52840_serial
Jul 18, 2018
Merged

Fix nrf52840 serial mentioned #1021#1036
arturo182 merged 4 commits into
adafruit:masterfrom
hathach:fix_nrf52840_serial

Conversation

@hathach

@hathach hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member
  • update tinyusb for wanted char
  • move usb code into usb.c

hathach added 2 commits July 17, 2018 21:24
@hathach
should fix adafruit#1021
3525d65 
- update tinyusb for wanted char
- move usb code into usb.c
@hathach
Merge branch 'master' into fix_nrf52840_serial
d4e84a9
@hathach

hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

@hathach
use seperate interrupt_char.c for nrf
a0b9871 
move usb descriptors into usb.c
@hathach

hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

should be OK now. I am not sure if the lib/utils/interrupt_char.c is OK to edit. So I add a separated interrupt_char.c with modification under nrf/ . It only need to tell the usb stack to trigger callback when receiving the Ctrl+C

@hathach hathach changed the title Fix nrf52840 serial Fix nrf52840 serial mentioned #1021 Jul 17, 2018
@hathach

hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

#1021

arturo182
arturo182 suggested changes Jul 17, 2018
View reviewed changes

@arturo182 arturo182 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks ok, even though we have to keep our own copy of interrupt_char.c now, but I have a few other comments.

Comment thread ports/nrf/usb/usb.h Outdated
#ifndef MICROPY_INCLUDED_NRF_USB_H
#define MICROPY_INCLUDED_NRF_USB_H

#ifdef NRF52840_XXAA

@arturo182 arturo182 Jul 17, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

Comment thread ports/nrf/usb/usb.c Outdated
#include "usb.h"
#include "lib/utils/interrupt_char.h"

#ifdef NRF52840_XXAA

@arturo182 arturo182 Jul 17, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

Comment thread ports/nrf/usb/usb.c

void usb_init(void) {

#ifdef SOFTDEVICE_PRESENT

@arturo182 arturo182 Jul 17, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this ifdef is needed, I did the fix for having usb and SD co-exist and no extra code is needed here.

@hathach hathach Jul 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the part when SD is enabled is currently handled inside the stack.
https://github.com/hathach/tinyusb/blob/develop/src/portable/nordic/nrf5x/hal_nrf5x.c#L178

Previously they are all inside in tusb hal, then recent Nordic sdk break non-SD api by introducing nrfx (SD API is more stable). But I think I will move it out of the stack, making the stack more generic. I will take this chance to do this

@hathach hathach Jul 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all is planned here :D :D
https://github.com/hathach/tinyusb/blob/develop/src/portable/nordic/nrf5x/hal_nrf5x.c#L180

@hathach

hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

@arturo182 do we define SOFTDEVICE_PRESENT when SD is selected, I couldn't find it within the makefile

@arturo182

arturo182 commented Jul 17, 2018

Copy link
Copy Markdown
Collaborator

It actually looks like we don't define it, it's defined usually in the Nordic SDK which we don't use. Should probably define it in the Makefile if SD is not empty, because even nRFx checks for it sometimes.

@hathach

hathach commented Jul 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@arturo182 it is used throughout Nordic code, it is good idea to use it to detect SD existence as well. I think for peripheral that is shared with SD, nrfx will need that to detect if the SD existed.

@hathach

hathach commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

btw, could you brief me on how to run a simple ble script e.g advertising something. I will check if changes break anything when the SD is running.

@arturo182

arturo182 commented Jul 17, 2018

Copy link
Copy Markdown
Collaborator

For sure!
The simplest way I can think of to check if BLE works is:

import ubluepy
p = ubluepy.Peripheral()
p.advertise(device_name='Python')

After that you should be able to see a device named Python when scanning, I use https://play.google.com/store/apps/details?id=no.nordicsemi.android.mcp&hl=en to test it.

tannewt
tannewt approved these changes Jul 17, 2018
View reviewed changes

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @hathach.

@arturo182 can you merge after you approve?

@arturo182

arturo182 commented Jul 17, 2018

Copy link
Copy Markdown
Collaborator

@tannewt I can approve but not merge:

Only those with write access to this repository can merge pull requests.

@arturo182 arturo182 merged commit 1fb8197 into adafruit:master Jul 18, 2018
@hathach hathach deleted the fix_nrf52840_serial branch July 20, 2018 07:42
@bobbahatti

bobbahatti commented Jan 21, 2024

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tannewt tannewt tannewt approved these changes
@jerryneedell jerryneedell Awaiting requested review from jerryneedell
@dhalbert dhalbert Awaiting requested review from dhalbert
+1 more reviewer
@arturo182 arturo182 arturo182 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hathach @arturo182 @bobbahatti @tannewt