Skip to content

Fix nrf52840 serial mentioned #1021 #1036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 18, 2018
Merged

Fix nrf52840 serial mentioned #1021 #1036

merged 4 commits into from
Jul 18, 2018

Conversation

hathach
Copy link
Member

@hathach hathach commented Jul 17, 2018

  • 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
Copy link
Member Author

hathach commented Jul 17, 2018

sorry there is a bit off issue, the current code will swallow the ctrl+c as well, hold on, I will fix it.

@hathach
use seperate interrupt_char.c for nrf
a0b9871 
move usb descriptors into usb.c
@hathach
Copy link
Member Author

hathach commented Jul 17, 2018

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
Copy link
Member Author

hathach commented Jul 17, 2018

#1021

arturo182
arturo182 requested changes Jul 17, 2018
View reviewed changes
Copy link
Collaborator

@arturo182 arturo182 left a comment

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.

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

#ifdef NRF52840_XXAA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove

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

#ifdef NRF52840_XXAA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove

ports/nrf/usb/usb.c Outdated

void usb_init(void) {

#ifdef SOFTDEVICE_PRESENT
Copy link
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.

Copy link
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

Copy link
Member Author

@hathach hathach Jul 17, 2018 •
edited
Loading

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
Copy link
Member Author

hathach commented Jul 17, 2018

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

@arturo182
Copy link
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
Copy link
Member Author

hathach commented Jul 17, 2018 •
edited
Loading

@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
Copy link
Member Author

hathach commented Jul 17, 2018

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
Copy link
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
Copy link
Member

@tannewt tannewt left a comment

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
Copy link
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
Copy link

https://play.google.com/store/apps/details?id=no.nordicsemi.android.mcp

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

@arturo182 arturo182 arturo182 approved these changes

@jerryneedell jerryneedell Awaiting requested review from jerryneedell

@dhalbert dhalbert Awaiting requested review from dhalbert

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