Thingy91 USB-UART bridge #1011

bjda · 2019-07-31T15:59:51Z

This PR contains the firmware intended to run on the nRF52840 on the Thingy:91, and updated board support for the same.

CLAassistant · 2019-07-31T15:59:59Z

All committers have signed the CLA.

carlescufi · 2019-08-01T09:08:33Z

I wonder if this should be in applications/thingy91/usb_uart_bridge instead, since it seems specific to that board. If this is to become a generic USB to UART bridge for any board, and will be configurable to work on any board (for example 52840), then we can leave it where it is.

bjda · 2019-08-01T09:32:09Z

@carlescufi It was developed for the Thingy:91, but the code itself is generic, as long as the device is Nordic, and has a UART and USB device. It should be configurable for something like the nRF52840 DK or dongle down the line.

jhn-nordic

Have been proven to work on the Thingy:91 for a while now.

lemrey

Looks okay, left a few comments. If this can indeed run on other boards, I think it should be put under /samples rather than /applications.

applications/usb_uart_bridge/src/main.c

bjda · 2019-09-02T16:19:24Z

@lemrey could you revisit?

lemrey

Looks better, left a few comments (remember to squash the commits).
You should add a yml file to the project directory and we should decide on the location.
If you say this will support more than one board in the future it would make sense to move it under samples, otherwise we can leave it here. I think it would be useful to have it run on another board.

applications/usb_uart_bridge/src/main.c

applications/usb_uart_bridge/prj.conf

bjda · 2019-09-12T14:05:05Z

@lemrey All right, handled the comments. Regarding the location in NCS, these are my thoughts after some deliberation:

I see two development paths for this app:

Adding more functionality specific to the shipped Thingy:91 firmware (combining UART bridge, BLE HCI, using the onboard SWD interface, extra I/O, maybe more). This would probably make sense to put in something like applications/thingy91/thingy91_interface, so as not to restrict it to a UART bridge by name.
Adding configuration support for more boards as a generic USB-UART bridge. This would make more sense to put in samples.

There is value in both options, but I propose we start with applications/thingy91/thingy91_interface for now. That will allow the Thingy:91 project to start building product releases for the nRF52 from a consistent location in the NCS.

Since a UART bridge is generally useful for other projects or a standalone sample, maybe we should over time make a configurable subsystem for it (not unlike the at_host library) with its own sample.

Would be great with some input from @jtguggedal and/or @joakimtoe as well on this.

applications/usb_uart_bridge/sample.yaml

lemrey

I am fine with leaving it under /applications if we don't want to support other boards for now.
Do you plan to move this to /applications/thingy91/... ?

NordicBuilder · 2019-09-16T16:54:35Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

bjda · 2019-09-16T17:18:14Z

@lemrey Handled comments and moved to applications/thingy91/usb_uart_bridge

Also increased the power management thread stack up a bit (to CONFIG_IDLE_STACK_SIZE) as this fixed some crashes. The rationale behind using that macro is that it is the configured stack size for doing almost nothing except being ready for interrupts/preemption/logging/etc, which matches the power thread.

lemrey

Thank you for addressing all the comments 👍

lemrey · 2019-09-17T08:08:05Z

Added entry to changelog.

rlubos

Looks fine, one nitpick to fix.

applications/thingy91/usb_uart_bridge/src/main.c

carlescufi

This requires a bit of documentation, a bare minimum at least.
Also I would move it to applications/usb/usb_uart_bridge

applications/thingy91/usb_uart_bridge/src/main.c

jtguggedal

One question about license, otherwise looks good to me.
I've been running the application reliably for several hours without issues. MCUboot part of it has not been tested, though.

bjda · 2019-09-18T15:37:24Z

Added doc and moved to samples, as per internal discussion

carlescufi

LGTM, thanks for your patience

jtguggedal · 2019-09-18T20:04:11Z

One last nit: After moving it to samples, the commit title could be changed to reflect that

Adjust board configurations to be similar to PCA10059, which also uses MCUboot over CDC ACM. Signed-off-by: Jan Tore Guggedal <jantore.guggedal@nordicsemi.no>

Adds USB to UART bridge intended for PCA20035. Signed-off-by: Jan Tore Guggedal <jantore.guggedal@nordicsemi.no> Signed-off-by: Bernt Johan Damslora <bernt.johan.damslora@nordicsemi.no>

bjda · 2019-09-19T15:15:13Z

Fixed missing entry in TOC and commit name. Should be ready to merge now

bjda requested review from anangl, carlescufi and ioannisg as code owners July 31, 2019 15:59

nrfconnect deleted a comment from CLAassistant Aug 1, 2019

carlescufi requested review from jtguggedal, pdunaj and lemrey August 1, 2019 09:07

jhn-nordic approved these changes Aug 15, 2019

View reviewed changes

lemrey reviewed Aug 16, 2019

View reviewed changes

bjda force-pushed the pca20035_bridge branch from fad97f5 to 78d5d3c Compare September 2, 2019 15:59

lemrey reviewed Sep 4, 2019

View reviewed changes

bjda force-pushed the pca20035_bridge branch 2 times, most recently from 0a58f7d to d464df9 Compare September 12, 2019 10:15

lemrey reviewed Sep 16, 2019

View reviewed changes

applications/usb_uart_bridge/sample.yaml Outdated Show resolved Hide resolved

lemrey reviewed Sep 16, 2019

View reviewed changes

bjda force-pushed the pca20035_bridge branch from d464df9 to 7b73783 Compare September 16, 2019 16:52

bjda force-pushed the pca20035_bridge branch from 7b73783 to ba617a6 Compare September 16, 2019 17:01

bjda requested a review from lemrey September 16, 2019 17:18

lemrey approved these changes Sep 17, 2019

View reviewed changes

rlubos approved these changes Sep 17, 2019

View reviewed changes

applications/thingy91/usb_uart_bridge/src/main.c Outdated Show resolved Hide resolved

applications/thingy91/usb_uart_bridge/src/main.c Outdated Show resolved Hide resolved

bjda force-pushed the pca20035_bridge branch from ba617a6 to ec96c44 Compare September 17, 2019 16:33

carlescufi requested changes Sep 18, 2019

View reviewed changes

jtguggedal reviewed Sep 18, 2019

View reviewed changes

applications/thingy91/usb_uart_bridge/src/main.c Outdated Show resolved Hide resolved

jtguggedal approved these changes Sep 18, 2019

View reviewed changes

bjda force-pushed the pca20035_bridge branch from ec96c44 to dd55987 Compare September 18, 2019 15:34

bjda force-pushed the pca20035_bridge branch from dd55987 to 143830a Compare September 18, 2019 15:48

carlescufi approved these changes Sep 18, 2019

View reviewed changes

bjda force-pushed the pca20035_bridge branch from 143830a to 768a5e3 Compare September 19, 2019 09:31

bjda requested a review from ru-fu as a code owner September 19, 2019 09:31

bjda force-pushed the pca20035_bridge branch 2 times, most recently from a207fd7 to a622751 Compare September 19, 2019 10:37

jtguggedal and others added 2 commits September 19, 2019 16:29

boards: nrf52840: Adjust board configuration for MCUboot

cf239f9

Adjust board configurations to be similar to PCA10059, which also uses MCUboot over CDC ACM. Signed-off-by: Jan Tore Guggedal <jantore.guggedal@nordicsemi.no>

samples: Add USB to UART bridge

295ea55

Adds USB to UART bridge intended for PCA20035. Signed-off-by: Jan Tore Guggedal <jantore.guggedal@nordicsemi.no> Signed-off-by: Bernt Johan Damslora <bernt.johan.damslora@nordicsemi.no>

bjda force-pushed the pca20035_bridge branch from a622751 to 295ea55 Compare September 19, 2019 14:29

bjda requested a review from carlescufi September 19, 2019 15:14

carlescufi merged commit f4c2c52 into nrfconnect:master Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thingy91 USB-UART bridge #1011

Thingy91 USB-UART bridge #1011

bjda commented Jul 31, 2019

CLAassistant commented Jul 31, 2019 •

edited

carlescufi commented Aug 1, 2019

bjda commented Aug 1, 2019

jhn-nordic left a comment

lemrey left a comment •

edited

bjda commented Sep 2, 2019

lemrey left a comment •

edited

bjda commented Sep 12, 2019

lemrey left a comment

NordicBuilder commented Sep 16, 2019 •

edited

bjda commented Sep 16, 2019

lemrey left a comment

lemrey commented Sep 17, 2019

rlubos left a comment

carlescufi left a comment

jtguggedal left a comment

bjda commented Sep 18, 2019

carlescufi left a comment

jtguggedal commented Sep 18, 2019

bjda commented Sep 19, 2019

Thingy91 USB-UART bridge #1011

Thingy91 USB-UART bridge #1011

Conversation

bjda commented Jul 31, 2019

CLAassistant commented Jul 31, 2019 • edited

carlescufi commented Aug 1, 2019

bjda commented Aug 1, 2019

jhn-nordic left a comment

Choose a reason for hiding this comment

lemrey left a comment • edited

Choose a reason for hiding this comment

bjda commented Sep 2, 2019

lemrey left a comment • edited

Choose a reason for hiding this comment

bjda commented Sep 12, 2019

lemrey left a comment

Choose a reason for hiding this comment

NordicBuilder commented Sep 16, 2019 • edited

bjda commented Sep 16, 2019

lemrey left a comment

Choose a reason for hiding this comment

lemrey commented Sep 17, 2019

rlubos left a comment

Choose a reason for hiding this comment

carlescufi left a comment

Choose a reason for hiding this comment

jtguggedal left a comment

Choose a reason for hiding this comment

bjda commented Sep 18, 2019

carlescufi left a comment

Choose a reason for hiding this comment

jtguggedal commented Sep 18, 2019

bjda commented Sep 19, 2019

CLAassistant commented Jul 31, 2019 •

edited

lemrey left a comment •

edited

lemrey left a comment •

edited

NordicBuilder commented Sep 16, 2019 •

edited