nrf52840: implement USB-CDC #883

syoder · 2020-01-28T18:07:31Z

This is based on the implementation in machine_atsamd51.go, with changes made to use the USBD registers found on the nRF52840.

syoder · 2020-01-28T18:08:45Z

This isn't done yet but I wanted to post what I've done so far in case anyone has bandwidth to test, provide feedback, or help finish it. I think it's getting close.

aykevl

A quick superficial review. I haven't done an in-depth review as that's hard to do with all the TODOs around, I assume at least some of those will get fixed anyway.

src/machine/usb_nrf52840.go

aykevl · 2020-01-28T18:23:18Z

src/machine/usb_nrf52840.go

+	// TODO: do a "real" conversion that supports more than just ascii chars
+	for i, char := range in {


Note that you're already decoding Unicode code points here: char is actually a single Unicode code point (and i is the index into the string, which will increase by more than 1 when decoding non-ASCII characters).

I just copied what was done here except I'm iterating runes instead of bytes. Neither this implementation or the other will work for anything except ascii characters. I was worried about pulling in the unicode package to convert, but it looks like I can do that fairly easily.

Oh, it was more of a hint. And yeah, if possible it would be best to avoid the unicode package (it is really big and currently slow to compile).

bgould · 2020-01-29T03:06:46Z

I have this board and I can try testing soon. Thanks for implementing this

syoder · 2020-01-29T20:45:00Z

I seem to be stuck. The device seems to connect to my Mac and shows up as a serial port that I can open via screen. So I know all the setup / enumeration is happening properly. However, it locks up my main program - the only thing that works is stuff that runs in the interrupts.

I'm having this same problem with a nRF PWM driver that I'm writing too. It seems like whenever I enable an interrupt (even if the interrupt function is an empty function), everything stops working.

In order to move this forward I think I need to connect a serial adapter to my board's UART pins so I can get some additional debugging output. I'll see if I can find something at my office tomorrow.

syoder · 2020-02-05T03:49:06Z

Questions that need to be answered:

Should everything in src/machine/usb_nrf52840.go be moved into src/machine/machine_nrf52840.go? I was looking for a way to break things out because those files are already so big, but 🤷‍♂
Should we set machine.UART0 to be this new USBCDC object so that putchar() automatically writes to it and debug output goes to it? Or should we leave it at machine.USB as I have it now?
This currently sends PID/VID and string descriptors as set in machine/usb.go which are for an Arduino Zero. It looks like machine/usb: set the vid and pid to valid values supplied by Adafruit and Arduino #884 refactors this a bit so that each board uses it's own - If that gets merged first, I should probably make a similar change here. If this gets merged first, that PR should include nrf52840 boards.
Same thing applies to fixing the way string descriptors are sent in the USB-CDC implementation for sam - if this gets merged first, the strToUTF16LEDescriptor function could be used there as well.

bgould · 2020-02-05T04:27:26Z

Should we set machine.UART0 to be this new USBCDC object so that putchar() automatically writes to it and debug output goes to it? Or should we leave it at machine.USB as I have it now?

For this one my vote would definitely be to make the USBCDC the default output.

aykevl · 2020-02-05T17:02:07Z

Should everything in src/machine/usb_nrf52840.go be moved into src/machine/machine_nrf52840.go? I was looking for a way to break things out because those files are already so big, but 🤷‍♂️

Considering how much code it is, it sounds good to keep it in a separate file (close to usb.go).

Should we set machine.UART0 to be this new USBCDC object so that putchar() automatically writes to it and debug output goes to it? Or should we leave it at machine.USB as I have it now?

That depends on the board. For the Bluefruit I think putchar should use USB-CDC while the pca10056 should use the UART (because that is what is normally connected).

syoder · 2020-02-05T18:48:20Z

That depends on the board. For the Bluefruit I think putchar should use USB-CDC while the pca10056 should use the UART (because that is what is normally connected).

What if we added something like this to machine_nrf.go:

type ByteWriter interface {
  func WriteByte(c byte) error
}

var DEFAULT_OUTPUT ByteWriter

Then putchar() could use machine.DEFAULT_OUTPUT which could be set differently per board, but would also allow a developer to easily switch to whichever UART/USB device they wanted.

bgould · 2020-02-06T14:04:19Z

I tested this on Circuit Playground Bluefruit and Adafruit CLUE boards, and it appears to work flawlessly so far.

With respect to the default output... I wonder if it might be enough to move the definition of the UART0 out of machine_nrf.go and into the individual board_*.go files? I think that could be addressed in a separate if you didn't want this one to get larger.

syoder · 2020-02-06T15:56:01Z

I wouldn't mind moving where UART0 gets defined to the board_*.go files in this PR.

After a quick review, it looks like these are the currently supported nRF52840 based boards:

Circuit Playground Bluefruit
reel board
nRF52840-mdk
PCA10056

Of those, I'm guessing CPB and nRF52840-mdk would probably want USB-CDC to be UART0, while reel board and PCA10056 would probably want the actual UART? This is all just based on pictures of the boards on the internet, I'm not that familiar with any of them except for CPB.

syoder · 2020-02-07T13:54:51Z

Looks like the smoke tests depend on UART0 being defined in the machine_*.go files and fail if you try to define them in board_*.go. Unless anyone can tell me how to avoid this issue, I'll revert my last commit and we'll have to deal with that part in another PR.

aykevl

Right. This breaks because of support for play.tinygo.org.
I would say that the appropriate solution is to create a new board_reelboard_baremetal.go (and similar) that contains the UART. It's not pretty, but that's how we worked around the problem in #766 for example.

aykevl · 2020-02-07T23:00:32Z

src/machine/board_pca10056.go

+// UART0 is the NRF UART and UART1 is the nRF52840's USB device
+var (
+	UART0 = NRF_UART0
+	UART1 = USB


The nrf52840 has an UART1 (actually UARTE1) so this would conflict.
I think it would be better to leave UART1 undefined and expect programs to use machine.USB if they specifically need to write to that output.
(The same is true for other boards that primarily use a real UART for output).

Should I also leave UART1 undefined for boards that will use USB as the primary output? (programs would have to access NRF_UART0 if they want to want to use the real UART)

Should I also leave UART1 undefined for boards that will use USB as the primary output? (programs would have to access NRF_UART0 if they want to want to use the real UART)

Not sure. @deadprogram any opinion on this?

So up until now, we have typically treated UART0 as either a "real" UART or the USB-CDC interface, depending on what the board supports.

However, that now seems a little clumsy. I think we should make a chance and instead do something like what we do with machine.LED to have a well-known alias. We could name it something like machine.SERIAL or machine.SERIALPORT.

Boards where there is a USB-CDC like basically all of the Adafruit boards, machine.SERIAL = machine.USB.

Boards where there the "default" is actually a UART perhaps via a debug chip. then machine.SERIAL = machine.UART0.

If the user code needs to communicate with a specific UART, then that is easily handled. The default is the "sensible" option. And we can define all of the available ports per board without having to leave something out, or renumber.

This sounds very reasonable. Completely agree.

That would also fix weird cases like this:

tinygo/src/machine/board_bluepill.go

Lines 62 to 68 in 1d913a6

// USART1 is the first hardware serial port on the STM32.

// Both UART0 and UART1 refer to USART1.

UART0 = UART{

Buffer: NewRingBuffer(),

Bus: stm32.USART1,

}

UART1 = &UART0

However, this machine.SERIAL (or maybe machine.Serial?) should probably be implemented in a separate PR.

machine.Serial seems good. Also sounds good to make those changes in a separate PR.

aykevl · 2020-02-07T23:07:41Z

After a quick review, it looks like these are the currently supported nRF52840 based boards:

Circuit Playground Bluefruit

reel board

nRF52840-mdk

PCA10056

I merged #893 so now there are five boards. This PR should be rebased on top of the dev branch to make sure that board is also included.

This is based on the implementation in machine_atsamd51.go, with changes made to use the USBD registers found on the nRF52840.

It turns out I needed to read the manual a little better in order to see the sequence of actions and events that the nrf USBD device expects.

Previously it was attempting to reset the register by setting it to zero, but the manual says to reset bits you need to set them to 1. Once this was changed, the device began properly acknowledging when it received data from the host on the out endpoint.

This change allows the `board_*.go` files to choose which device will be UART0 and which will be UART1. For some it makes sense for the USB connection to be UART0 and for others it may not.

Create separate board_*_baremetal.go files for reelboard and pca10056 which will define UART0, in order to continue to allow support for these with the tinygo playground. Also switches the adafruit clue board to use the USB as UART0.

...and to remove confusion with the UARTE1 device.

deadprogram · 2020-02-17T06:23:32Z

Is there anything else outstanding on this PR before it can be merged? It would appear to be ready.

suda · 2020-02-17T12:42:34Z

#904 will add three more nRF52 based boards, all of them default UART0 to be the CDC and UART1 being the device's TX and RX pins so definitely a +1 on UART0 being USB-CDC.

syoder · 2020-02-17T14:00:19Z

As far as I know it's ready. Unless someone tells me something needs to be changed.

deadprogram · 2020-02-17T14:12:46Z

OK, lets merge then. Massive thanks to @syoder for all your work on this, and also to everyone who helped test, provide feedback, and otherwise help make it happen!

aykevl reviewed Jan 28, 2020

View reviewed changes

syoder force-pushed the usb-cdc-nrf52840 branch from b483232 to a71c9f3 Compare January 29, 2020 20:38

syoder force-pushed the usb-cdc-nrf52840 branch from c8c8e39 to efbff51 Compare February 5, 2020 03:35

syoder marked this pull request as ready for review February 5, 2020 03:50

syoder requested a review from aykevl February 5, 2020 03:51

syoder force-pushed the usb-cdc-nrf52840 branch from 82a893c to 8b15a78 Compare February 7, 2020 03:10

aykevl reviewed Feb 7, 2020

View reviewed changes

syoder added 9 commits February 7, 2020 19:33

usb-cdc implementation for nRF52840

7c9cb6d

This is based on the implementation in machine_atsamd51.go, with changes made to use the USBD registers found on the nRF52840.

use new interrupt framework; convert to utf16le descriptors

8b281ae

move interrupt to a member of the USBCDC struct

4b12633

properly reset the USBEVENT interrupt event

c026ffa

fix reading data coming from host

0f8c456

It turns out I needed to read the manual a little better in order to see the sequence of actions and events that the nrf USBD device expects.

make handleEndpoint not rely on global variable

53ab2c2

set UART0 to be the USB-CDC on some nRF52840 boards

ddb311f

This change allows the `board_*.go` files to choose which device will be UART0 and which will be UART1. For some it makes sense for the USB connection to be UART0 and for others it may not.

work around issue with support for play.tinygo.org

a3dc24a

Create separate board_*_baremetal.go files for reelboard and pca10056 which will define UART0, in order to continue to allow support for these with the tinygo playground. Also switches the adafruit clue board to use the USB as UART0.

syoder force-pushed the usb-cdc-nrf52840 branch from 8b15a78 to a3dc24a Compare February 8, 2020 03:02

remove UART1 from board nrf52840-mdk to make things consistent

6fd0c10

...and to remove confusion with the UARTE1 device.

deadprogram mentioned this pull request Feb 9, 2020

machine/usb: set the vid and pid to valid values supplied by Adafruit and Arduino #884

Merged

properly receive and parse usb line info (baud rate, etc)

1a09af5

syoder force-pushed the usb-cdc-nrf52840 branch from af23e9b to 1a09af5 Compare February 10, 2020 01:17

bgould mentioned this pull request Feb 10, 2020

1200 baud auto-reset over USB for nrf52840 #898

Merged

deadprogram merged commit 630c498 into tinygo-org:dev Feb 17, 2020

suda mentioned this pull request Feb 18, 2020

Add support for Particle Argon, Boron and Xenon boards #904

Merged

		// TODO: do a "real" conversion that supports more than just ascii chars
		for i, char := range in {

	// USART1 is the first hardware serial port on the STM32.
	// Both UART0 and UART1 refer to USART1.
	UART0 = UART{
	Buffer: NewRingBuffer(),
	Bus: stm32.USART1,
	}
	UART1 = &UART0

nrf52840: implement USB-CDC #883

nrf52840: implement USB-CDC #883

Uh oh!

Conversation

syoder commented Jan 28, 2020

Uh oh!

syoder commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgould commented Jan 29, 2020

Uh oh!

syoder commented Jan 29, 2020

Uh oh!

syoder commented Feb 5, 2020

Uh oh!

bgould commented Feb 5, 2020

Uh oh!

aykevl commented Feb 5, 2020

Uh oh!

syoder commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgould commented Feb 6, 2020

Uh oh!

syoder commented Feb 6, 2020

Uh oh!

syoder commented Feb 7, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aykevl Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aykevl commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deadprogram commented Feb 17, 2020

Uh oh!

suda commented Feb 17, 2020

Uh oh!

syoder commented Feb 17, 2020

Uh oh!

deadprogram commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

syoder commented Jan 28, 2020 •

edited

Loading

syoder commented Feb 5, 2020 •

edited

Loading

aykevl Feb 9, 2020 •

edited

Loading

aykevl commented Feb 7, 2020 •

edited

Loading