machine/usb: descriptor refactor #3636

deadprogram · 2023-04-02T10:23:00Z

This PR is to refactor the USB descriptors to make them modular. This is in part to address what is being discussed in #3474

There is still further work needed to complete this feature, this PR in intended to continue the conversation in the form of code.

deadprogram · 2023-04-02T10:29:13Z

cc @neildavis

deadprogram · 2023-04-02T10:34:24Z

Note also this PR includes #3626

deadprogram · 2023-04-03T18:10:25Z

This PR now has the mostly complete refactoring of the USB descriptors. The only parts that are still incomplete are to finish creating types for HID keyboard and mouse descriptors.

EDIT: I added comments to the HID reports, that is probably enough for now.

deadprogram · 2023-04-03T19:55:05Z

PTAL @bgould @ardnew @sago35 and anyone else who might be interested.

deadprogram · 2023-04-04T17:36:16Z

Had to do one last thing on this PR, which was to cleanup usb.EnableJoystick() so it would use the proper method to change the ClassLength instead of slice modification magic.

sago35 · 2023-04-06T23:25:22Z

I have started reviewing the code. I think it is good work. On the other hand, I may have to think a little more when adding new devices such as msd.

However, I still think it is a very good improvement.

sago35 · 2023-04-09T02:51:36Z

src/machine/usb/descriptor/midi.go

+const endpointMIDITypeLen = 9
+
+var endpointEP6IN = [endpointMIDITypeLen]byte{
+	endpointMIDITypeLen,
+	TypeEndpoint,
+	0x86, // EndpointAddress
+	0x02, // Attributes
+	0x40, // MaxPacketSizeL
+	0x00, // MaxPacketSizeH
+	0x00, // Interval
+	0x00, // refresh
+	0x00, // sync address
+}
+
+var EndpointEP6IN = EndpointType{
+	data: endpointEP6IN[:],
+}
+
+var endpointEP7OUT = [endpointMIDITypeLen]byte{
+	endpointMIDITypeLen,
+	TypeEndpoint,
+	0x07, // EndpointAddress
+	0x02, // Attributes
+	0x40, // MaxPacketSizeL
+	0x00, // MaxPacketSizeH
+	0x00, // Interval
+	0x00, // refresh
+	0x00, // sync address
+}
+
+var EndpointEP7OUT = EndpointType{
+	data: endpointEP7OUT[:],
+}


Shouldn't it be implemented in endpoint.go?

I was thinking about that, however since these endpoints have values specific to the MIDI protocol (note the length difference).

Perhaps they should have MIDI in the endpoint name to make that more explicit?

sago35 · 2023-04-11T12:15:19Z

midi and joystick may not be working.

deadprogram · 2023-04-11T12:39:32Z

midi and joystick may not be working.

I will check that out later today, thanks for letting me know.

deadprogram · 2023-04-12T19:28:11Z

@sago35 I tested this branch, and was not able to get keyboard or MIDI to work using circuitplayground express board.

Then I tested the same thing on the dev branch, and again I was not able to get either keyboard or MIDI to work.

Finally I tested the last release, and it also did not work ☹️

Are you having this same problem with your devices? Which processor are you using?

deadprogram · 2023-04-12T19:50:49Z

OK, checked out code using Clue (nrf52840)

In this branch using Clue the Keyboard works, however MIDI does not. Have not tried Joystick yet.

deadprogram · 2023-04-14T08:10:35Z

@sago35 did some more testing last night, appears the dev branch is not working on rp2040 or samd21 when I tested using the usb-midi example.

The release branch still working as expected when I plugged both devices into my Macbook. However dev not working for either board. Looks like we will need to bisect changes to locate the cause.

deadprogram · 2023-04-14T22:51:10Z

Did a git bisect and found that f41b6a3 is the commit where this stopped working. Looking into where we are doing the allocation that is causing this problem now...

deadprogram · 2023-04-15T18:25:47Z

DId a session with gdb and an ItsyBitsy-M4. The results were not what I expected:

$ tinygo gdb -target itsybitsy-m4 -programmer=jlink examples/usb-midi                                                                                                        
GNU gdb (Ubuntu 9.2-0ubuntu1~20.04.1) 9.2                                                                                                                                    
Copyright (C) 2020 Free Software Foundation, Inc.                                                                                                                            
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>                                                                                                
This is free software: you are free to change and redistribute it.                                                                                                           
There is NO WARRANTY, to the extent permitted by law.                                                                                                                        
Type "show copying" and "show warranty" for details.                                                                                                                         
This GDB was configured as "x86_64-linux-gnu".                                                                                                                               
Type "show configuration" for configuration details.                                                                                                                         
For bug reporting instructions, please see:                                                                                                                                  
<http://www.gnu.org/software/gdb/bugs/>.                                                                                                                                     
Find the GDB manual and other documentation resources online at:                                                                                                             
    <http://www.gnu.org/software/gdb/documentation/>.                                                                                                                        
                                                                                                                                                                             
For help, type "help".                                                                                                                                                       
Type "apropos word" to search for commands related to "word"...                                                                                                              
Reading symbols from /tmp/tinygo2055889492/main...                                                                                                                           
Remote debugging using :3333                                                                                                                                                 
0x00000004 in ?? ()                                                                                                                                                          
Loading section .text, size 0xd9b0 lma 0x4000                                                                                                                                
Loading section .tinygo_stacksizes, size 0x4 lma 0x119b0                                                                                                                     
Loading section .data, size 0x68c lma 0x119b4                                                                                                                                
Start address 0x00007638, load size 57408                                                                                                                                    
Transfer rate: 7007 KB/sec, 9568 bytes/write.                                                                                                                                
Expected an decimal digit (0-9)                                                                                                                                              
(gdb) bt                                                                                                                                                                     
#0  0x00007638 in Reset_Handler () at /home/ron/Development/tinygo/tinygo/src/runtime/panic.go:150

I get the same error in both the dev branch and this feature branch. Calling @aykevl to help on this one, please...

sago35 · 2023-04-16T06:00:48Z

@deadprogram @aykevl
I feel that this change ( f41b6a3 ) has a little too much impact.
For example, the tinygo-org/bluetooth code at hand did not work either.
Also, the joystick code no longer works.

Of course it is better to fix the heap allocation, but it takes time for the user to realize why it is not working.
In the next release, it may be better to keep warnings instead of errors, or to allow optin.

f41b6a3

kenbell · 2023-04-16T09:11:58Z

@deadprogram I think Ayke suggested setting this bp to try to catch it in the original PR: #1460

b runtime.runtimePanic

@sago35 I think we can make interrupt allocation checks either opt-in or opt-out fairly easily using build tags. The longer we leave it opt-in though, the more code will be written that does allocations inside interrupt handlers that needs fixing. Perhaps a plan like:

keep dev opt-out until we're close to next release
make it opt-in for a short period for the next release
make dev opt-out again
revisit at the subsequent release to see if we're ready to make the switch permanent / opt-out

deadprogram · 2023-04-16T10:02:26Z

I think Ayke suggested setting this bp to try to catch it in the original PR

@kenbell that was what was surprising, that the runtime panic was not that one, but was unsafeSlicePanic. So something else perhaps is going on? Not sure, which is why I think I need help from @aykevl to figure this out.

sago35 · 2023-04-16T10:40:49Z

My main concern is that the code that was working with tinygo0.27 will no longer work.
It would be best if we could warn them at the time of tinygo build.

deadprogram · 2023-04-16T11:04:10Z

I had forgotten to add the -opt=1 flag to my command, hence was not getting the correct debugging info before.

Here is debug info from commit f41b6a3

$ tinygo gdb -target itsybitsy-m4 -programmer=jlink -opt=1 examples/usb-midi                                                                                                 
GNU gdb (Ubuntu 9.2-0ubuntu1~20.04.1) 9.2                                                                                                                                    
Copyright (C) 2020 Free Software Foundation, Inc.    
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>         
This is free software: you are free to change and redistribute it.                    
There is NO WARRANTY, to the extent permitted by law.                                                    
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".                                                                         
Type "show configuration" for configuration details.                                                                   
For bug reporting instructions, please see:                                                              
<http://www.gnu.org/software/gdb/bugs/>.                                                                 
Find the GDB manual and other documentation resources online at:                                                                                                             
    <http://www.gnu.org/software/gdb/documentation/>.                                                                                                                        
                                                                                                         
For help, type "help".                                                                                                                                                       
Type "apropos word" to search for commands related to "word"...                                                        
Reading symbols from /tmp/tinygo1796678531/main...                                                                     
Remote debugging using :3333         
0xdeadbeee in ?? ()                                                                                                                                                                                                
Loading section .text, size 0x3a90 lma 0x4000                                                                                                                                                                      
Loading section .tinygo_stacksizes, size 0x4 lma 0x7a90                               
Loading section .data, size 0x13c lma 0x7a94                                                                                                                                                                       
Start address 0x00005e8a, load size 15312
Transfer rate: 2136 KB/sec, 5104 bytes/write.                                                                          
Expected an decimal digit (0-9)      
(gdb) b runtime.runtimePanic                        
Breakpoint 1 at 0x51d4                              
(gdb) bt                                                                                                 
#0  0x00005e8a in runtime.preinit () at /home/ron/Development/tinygo/tinygo/src/machine/usb/cdc/usbcdc.go:114                                                                                                                                 
#1  Reset_Handler () at /home/ron/Development/tinygo/tinygo/src/runtime/runtime_atsamd51.go:19                                                                                                                                                
(gdb) f                                             
#0  0x00005e8a in runtime.preinit () at /home/ron/Development/tinygo/tinygo/src/machine/usb/cdc/usbcdc.go:114                                                                                                                                 
114             usbcdc.Write([]byte{c})                    
(gdb) l                                                    
109             return len(data), nil                      
110     }                                                  
111                                                        
112     // WriteByte writes a byte of data to the USB CDC interface.                                                   
113     func (usbcdc *USBCDC) WriteByte(c byte) error {                                                                
114             usbcdc.Write([]byte{c})                    
115             return nil                                 
116     }                                                  
117                                                        
118     func (usbcdc *USBCDC) DTR() bool {                 
(gdb) c                                                    
Continuing.                                                

Program received signal SIGTRAP, Trace/breakpoint trap.                                                                
0xdeadbeee in ?? ()                                        
(gdb) bt                                                   
#0  0xdeadbeee in ?? ()                                    
Backtrace stopped: previous frame identical to this frame (corrupt stack?)                                             
(gdb) f                                                    
#0  0xdeadbeee in ?? ()

deadprogram · 2023-04-16T11:29:42Z

Played around a bit more. By commenting out this line https://github.com/tinygo-org/tinygo/blob/dev/src/examples/usb-midi/main.go#L23 the example works on dev branch.

So I think this branch with the refactor has an error, but there is separately something in dev branch that does not allow using fmt package from the handler function.

deadprogram · 2023-04-16T23:12:53Z

Had to make a few more changes in descriptors, but now Keyboard, Midi, and Joystick all appear to be working.

I will submit a separate PR to remove fmt from examples/usb-midi.

Now ready for "final" review.

deadprogram · 2023-04-16T23:25:44Z

Added #3660 to correct example as mentioned.

sago35 · 2023-04-17T10:26:19Z

I believe the code is fine.
But as far as I have tested, it works fine except for the pattern using joystick.UseSettings().
I will investigate a little more.

deadprogram · 2023-04-17T10:57:44Z

@sago35 can you please share the repo with the code in which you are using joystick.UseSettings() so I can take a look?

sago35 · 2023-04-17T11:00:30Z

@deadprogram
I am thinking that perhaps UseSettings is not being changed properly.
https://github.com/sago35/pico-ffb-steering-wheel/tree/7a1098872fdc3fb6f3747a69daca0c02974097f9

deadprogram · 2023-04-17T11:15:21Z

Just wondering why you need this code exactly? https://github.com/sago35/pico-ffb-steering-wheel/blob/7a1098872fdc3fb6f3747a69daca0c02974097f9/pid/pid_handler.go#L173-L195

I don't know a lot about HID joysticks, perhaps you can briefly explain what that code is doing?

sago35 · 2023-04-17T11:56:11Z

(gdb) b runtime.slicePanic
Breakpoint 1 at 0x10004db4: file C:\tinygo\tinygo\src\runtime/panic.go, line 137.
Note: automatically using hardware breakpoints for read-only addresses.
(gdb) c
Continuing.

Breakpoint 1, 0x10004db4 in runtime.slicePanic () at C:\tinygo\tinygo\src\runtime/panic.go:137
137             runtimePanic("slice out of range")
(gdb) bt
#0  0x10004db4 in runtime.slicePanic () at C:\tinygo\tinygo\src\runtime/panic.go:137
#1  0x10005f00 in runtime.run$1 () at C:\tinygo\tinygo\src\machine\usb\descriptor/hid.go:82
#2  0x10005bf2 in <goroutine wrapper> () at C:\tinygo\tinygo\src\runtime/scheduler_any.go:23
#3  0x100002fc in tinygo_startTask () at C:\\tinygo\\tinygo\\src\\internal\\task/task_stack_cortexm.S:26

sago35 · 2023-04-17T12:03:26Z

It appears that -1 is returned here and runtime.slicePanic is called. I will look into it more tomorrow.

tinygo/src/machine/usb/descriptor/hid.go

Line 80 in 8355ebc

idx := bytes.Index(data, section)

deadprogram · 2023-04-17T12:39:41Z

It appears that -1 is returned here and runtime.slicePanic is called. I will look into it more tomorrow.

I think that happened because the Joystick had already been enabled by the default. I just added some code to handle that case, please give it a try.

sago35 · 2023-04-18T23:43:52Z

src/machine/usb/descriptor/hid.go

+
+	// search only for ClassHIDType without the ClassLength,
+	// in case it has already been set.
+	idx := bytes.Index(data[:ClassHIDTypeLen-2], section)


ClassHIDTypeLen is 9, so this will not work.

Suggested change

idx := bytes.Index(data[:ClassHIDTypeLen-2], section)

idx := bytes.Index(data, section)

Oops, I had it reversed. Renamed the parameter names and corrected, seems to be working now.

sago35 · 2023-04-18T23:46:32Z

src/machine/usb.go

-	})
-	binary.LittleEndian.PutUint16(usb.DescriptorCDCJoystick.Configuration[idx+7:idx+9], uint16(len(hidDesc)))
-	usb.DescriptorCDCJoystick.HID[2] = hidDesc
+	class, err := descriptor.FindClassHIDType(descriptor.CDCJoystick.Configuration, descriptor.ClassHIDJoystick.Bytes())


EnableJoystick has been called twice, and the second call seems to fail to find it because ClassLength has already been set.
Why it is being called twice requires a little more checking.

sago35 · 2023-04-18T23:47:38Z

@deadprogram
Please rebase to the latest dev branch.

deadprogram · 2023-04-22T06:13:39Z

Please rebase to the latest dev branch.

Done.

deadprogram · 2023-04-25T17:39:08Z

@sago35 I think this PR is now ready, please take another look.

deadprogram · 2023-04-26T18:06:45Z

@sago35 rebased just now again against the latest dev.

Signed-off-by: deadprogram <ron@hybridgroup.com>

…t HID descriptor Signed-off-by: deadprogram <ron@hybridgroup.com>

sago35

LGTM

The following example and the FFB JoyStick also worked.

$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/echo
$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/hid-keyboard
$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/hid-mouse
$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/usb-midi
$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/hid-joystick

FFB JoyStick
https://github.com/sago35/pico-ffb-steering-wheel/tree/work

deadprogram · 2023-04-28T13:15:47Z

Thank you very much @sago35 and @kenbell for all of your assistance! Now merging.

tuffrabit · 2023-06-18T18:40:11Z

LGTM

The following example and the FFB JoyStick also worked.
$ tinygo flash --target feather-rp2040 --size short --monitor ./src/examples/hid-joystick

Hey! The hid-joystick example worked for you on a rp2040 board? I can get it to compile but it does not work. Windows 10 won't even enumerate it after flashing. It consistently gets either a error code 43 or 10 in the device manager. You actually got it to build, flash, enumerate, and then generate input in joy.cpl or something similar?

This is on 0.28.1 btw

deadprogram requested a review from sago35 April 2, 2023 10:23

deadprogram force-pushed the usb-descriptor-improvements branch 3 times, most recently from 273a5ce to acad29d Compare April 3, 2023 18:08

deadprogram marked this pull request as ready for review April 3, 2023 18:08

deadprogram requested a review from bgould April 4, 2023 10:29

deadprogram force-pushed the usb-descriptor-improvements branch 2 times, most recently from f270f00 to b1cf4b2 Compare April 4, 2023 17:32

deadprogram changed the title ~~machine/usb: descriptor improvements~~ machine/usb: descriptor refactor Apr 5, 2023

sago35 reviewed Apr 9, 2023

View reviewed changes

deadprogram force-pushed the usb-descriptor-improvements branch from b1cf4b2 to 63fa0ac Compare April 16, 2023 14:35

deadprogram mentioned this pull request Apr 16, 2023

examples: replace fmt with encoding/hex in usb-midi example #3660

Merged

deadprogram force-pushed the usb-descriptor-improvements branch from 2ad9752 to 8355ebc Compare April 17, 2023 08:27

sago35 reviewed Apr 18, 2023

View reviewed changes

deadprogram force-pushed the usb-descriptor-improvements branch from 79ca15a to faee2e4 Compare April 22, 2023 06:10

deadprogram added this to the v0.28.0 milestone Apr 23, 2023

deadprogram force-pushed the usb-descriptor-improvements branch from faee2e4 to 991d6b2 Compare April 24, 2023 19:26

deadprogram force-pushed the usb-descriptor-improvements branch from 991d6b2 to 7b94867 Compare April 26, 2023 18:05

deadprogram added 2 commits April 27, 2023 01:20

machine/usb: refactoring descriptors into subpackage for modularity

1c5c3fc

Signed-off-by: deadprogram <ron@hybridgroup.com>

machine/usb/hid/joystick: handle case where we cannot find the correc…

1a8e731

…t HID descriptor Signed-off-by: deadprogram <ron@hybridgroup.com>

deadprogram force-pushed the usb-descriptor-improvements branch from 7b94867 to 1a8e731 Compare April 26, 2023 23:21

sago35 approved these changes Apr 28, 2023

View reviewed changes

deadprogram merged commit 25b0341 into dev Apr 28, 2023
22 checks passed

deadprogram deleted the usb-descriptor-improvements branch April 28, 2023 13:15

tuffrabit mentioned this pull request Jun 19, 2023

tinygo v0.28.1 hid-joystick example not working tuffrabit/tinygo-questions#1

Open

sago35 mentioned this pull request Jun 20, 2023

machine/usb/hid,joystick: fix hidreport (3) #3802

Merged

	idx := bytes.Index(data[:ClassHIDTypeLen-2], section)
	idx := bytes.Index(data, section)

machine/usb: descriptor refactor #3636

machine/usb: descriptor refactor #3636

Conversation

deadprogram commented Apr 2, 2023

deadprogram commented Apr 2, 2023

deadprogram commented Apr 2, 2023

deadprogram commented Apr 3, 2023 • edited

deadprogram commented Apr 3, 2023

deadprogram commented Apr 4, 2023

sago35 commented Apr 6, 2023

sago35 Apr 9, 2023

Choose a reason for hiding this comment

deadprogram Apr 10, 2023

Choose a reason for hiding this comment

sago35 commented Apr 11, 2023

deadprogram commented Apr 11, 2023

deadprogram commented Apr 12, 2023 • edited

deadprogram commented Apr 12, 2023 • edited

deadprogram commented Apr 14, 2023

deadprogram commented Apr 14, 2023

deadprogram commented Apr 15, 2023

sago35 commented Apr 16, 2023 • edited

kenbell commented Apr 16, 2023

deadprogram commented Apr 16, 2023

sago35 commented Apr 16, 2023

deadprogram commented Apr 16, 2023

deadprogram commented Apr 16, 2023

deadprogram commented Apr 16, 2023

deadprogram commented Apr 16, 2023

sago35 commented Apr 17, 2023

deadprogram commented Apr 17, 2023

sago35 commented Apr 17, 2023

deadprogram commented Apr 17, 2023

sago35 commented Apr 17, 2023

sago35 commented Apr 17, 2023

deadprogram commented Apr 17, 2023

sago35 Apr 18, 2023

Choose a reason for hiding this comment

deadprogram Apr 24, 2023

Choose a reason for hiding this comment

sago35 Apr 18, 2023

Choose a reason for hiding this comment

sago35 commented Apr 18, 2023

deadprogram commented Apr 22, 2023

deadprogram commented Apr 25, 2023

deadprogram commented Apr 26, 2023

sago35 left a comment

Choose a reason for hiding this comment

deadprogram commented Apr 28, 2023

tuffrabit commented Jun 18, 2023

deadprogram commented Apr 3, 2023 •

edited

deadprogram commented Apr 12, 2023 •

edited

deadprogram commented Apr 12, 2023 •

edited

sago35 commented Apr 16, 2023 •

edited