Teensy 3.6 USB CDC #1223

firelizzard18 · 2020-07-09T05:56:40Z

USB CDC support for Teensy 3.6 (NXP MK66F18)

firelizzard18 · 2020-07-09T06:11:38Z

At one point, it looked like this version was working. Or at least I was seeing far more log messages from the USB ISR. But now it doesn't get past the device descriptor:

usb istat 0x01
usb istat 0x08
usb stat 0x00
usb bd tok 0xD
usb setup raw 8006000100004000
usb setup request=6 type=80 value=100 index=0
usb setup get descriptor
usb ep0 transmit 18 byte(s): 12010002ef020140c0168304000101020301
usb istat 0x08
usb stat 0x08
usb bd tok 0x9
usb istat 0x01

Except now, I'm not even getting that... All I get is "usb done" (indicating that machine.USB0.Configure() completed).

@deadprogram @aykevl Any ideas? As far as I can tell, things are failing early enough that Wireshark doesn't see anything.

aykevl

I never worked directly with USB-CDC so I don't think I can help you much debugging it, unfortunately.

(one quick comment below, I haven't really looked through the code)

aykevl · 2020-07-12T16:57:24Z

src/machine/board_teensy36.go

+	// for background about this startup delay, please see these conversations
+	// https://forum.pjrc.com/threads/36606-startup-time-(400ms)?p=113980&viewfull=1#post113980
+	// https://forum.pjrc.com/threads/31290-Teensey-3-2-Teensey-Loader-1-24-Issues?p=87273&viewfull=1#post87273
+
+	sleepTicks(25 * 1000) // 50 for TeensyDuino < 142
+	// usb_STRING_SERIAL = readSerialNumber()
+	USB0.Configure()
+	sleepTicks(275 * 1000) // 350 for TeensyDuino < 142


Is there a reason not to include this directly in postinit? It's not like you're supposed to call InitPlatform (except for the runtime).

I thought I would leave room for non-Teensy boards that are based on the NXP MK66F18. I believe these delays are more related to how the Teensy programmer detects boards than to the chip itself.

Ah, I see. So the function is only intended to be called from the runtime but put in the machine package because it is board specific? In that case, it would be useful to add a comment explaining that, so that it's clear this function isn't supposed to be called elsewhere.

aykevl

I'm not sure I understand the purpose of this PR. It doesn't seem to implement USB-CDC support, so what does it add in functionality?

aykevl · 2020-07-23T13:09:47Z

src/machine/board_teensy36.go

+	// for background about this startup delay, please see these conversations
+	// https://forum.pjrc.com/threads/36606-startup-time-(400ms)?p=113980&viewfull=1#post113980
+	// https://forum.pjrc.com/threads/31290-Teensey-3-2-Teensey-Loader-1-24-Issues?p=87273&viewfull=1#post87273
+
+	sleepTicks(25 * 1000) // 50 for TeensyDuino < 142
+	// usb_STRING_SERIAL = readSerialNumber()
+	USB0.Configure()
+	sleepTicks(275 * 1000) // 350 for TeensyDuino < 142


Ah, I see. So the function is only intended to be called from the runtime but put in the machine package because it is board specific? In that case, it would be useful to add a comment explaining that, so that it's clear this function isn't supposed to be called elsewhere.

aykevl · 2020-07-23T13:10:48Z

src/machine/usb_nxpmk66f18.go

+	"unsafe"
+)
+
+const debugUSB = true


Is this intentional? It looks like it should be false.

aykevl · 2020-07-23T13:13:01Z

src/machine/usb_nxpmk66f18.go

+
+import (
+	"device/nxp"
+	"fmt"


Don't import fmt in the machine package. This is a huge dependency with a large dependency graph and importing it in the machine package may lead to circular dependencies. It also bloats the binary and increases compile time.

Instead, I would suggest to use println for debugging (and disabled by default).

aykevl · 2020-07-23T13:15:37Z

src/machine/usb_nxpmk66f18.go

+	slice := (*reflect.SliceHeader)(unsafe.Pointer(&data))
+
+	*bde.retain = data
+	bde.record.address.Set(uint32(slice.Data))
+	bde.Describe(uint32(slice.Len), data1)


The reflect package is a relatively large dependency and shouldn't be used in the machine package. It is also possible to do without in this case:

Suggested change

slice := (*reflect.SliceHeader)(unsafe.Pointer(&data))

*bde.retain = data

bde.record.address.Set(uint32(slice.Data))

bde.Describe(uint32(slice.Len), data1)

*bde.retain = data

bde.record.address.Set(uint32(uintptr(unsafe.Pointer(&data[0]))))

bde.Describe(len(data), data1)

aykevl · 2020-07-23T13:19:29Z

src/machine/usb_nxpmk66f18.go

+	slice := (*reflect.SliceHeader)(unsafe.Pointer(&data))
+	slice.Len = uintptr(length)
+	slice.Cap = uintptr(length)
+	slice.Data = uintptr(bde.record.address.Get())


Where does bde.record.address come from? I'm guessing it is some sort of DMA buffer, did you put it in a global somewhere (for example) to make sure it is not garbage collected?
I think it would be much better to somehow keep the original slice around and use that, to avoid some unsafe code.

aykevl · 2020-07-23T13:23:31Z

src/machine/usb_nxpmk66f18.go

+	Log []string
+}
+
+func (u *USB) logf(format string, args ...interface{}) {
+	u.Log = append(u.Log, fmt.Sprintf(format, args...))
+}
+
+func (u *USB) debugf(format string, args ...interface{}) {
+	if debugUSB {
+		u.logf(format, args...)
+	}
+}
+
+func (u *USB) DumpLog() []string {
+	var log []string
+
+	state := interrupt.Disable()
+	log, u.Log = u.Log, nil
+	interrupt.Restore(state)
+
+	if log == nil {
+		return []string{}
+	}
+	return log
+}


This looks like debug code. While functions that aren't called won't take up extra space (they get optimized away), the Log buffer cannot be optimized away. Maybe you can make it a separate global (separate from the USB type) if you want to keep it?

aykevl · 2020-07-23T13:25:13Z

src/machine/usb_nxpmk66f18.go

+			Handshake: true,
+		})
+
+		// clear all ending interrupts


typo?

Suggested change

// clear all ending interrupts

// clear all pending interrupts

aykevl · 2020-07-23T13:27:23Z

src/machine/usb_nxpmk66f18.go

+}
+
+func (u USBCDC) WriteByte(byte) {
+	panic("not implemented")


This seems strange, you implement a USBCDC type that doesn't actually work?

aykevl · 2020-07-23T13:29:21Z

src/machine/usb_nxpmk66f18.go

+	usb_TOK_PID_SETUP = 0xD
+)
+
+type register8Aligned32 struct {


Just to be clear, this struct is not aligned on a 32-bit (4 byte) boundary. It is still 8 bit aligned but simply takes up four bytes. Was this intentional?

Initial Teensy USB support

06c1568

firelizzard18 force-pushed the teensy-3.6-cdc branch from 6e47b80 to 06c1568 Compare July 9, 2020 06:28

aykevl reviewed Jul 12, 2020

View reviewed changes

aykevl reviewed Jul 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teensy 3.6 USB CDC #1223

Teensy 3.6 USB CDC #1223

firelizzard18 commented Jul 9, 2020

firelizzard18 commented Jul 9, 2020

aykevl left a comment

aykevl Jul 12, 2020

firelizzard18 Jul 12, 2020

aykevl Jul 23, 2020

aykevl left a comment

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

aykevl Jul 23, 2020

	// clear all ending interrupts
	// clear all pending interrupts

Teensy 3.6 USB CDC #1223

Are you sure you want to change the base?

Teensy 3.6 USB CDC #1223

Conversation

firelizzard18 commented Jul 9, 2020

firelizzard18 commented Jul 9, 2020

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment