Teensyduino fork, need testers [Teensy 3.x]

Status
Not open for further replies.

xxxajk

Well-known member
The fork of the Teensyduino core HERE needs testing.

Fixes so far, that anyone with a Teensy 3.x can test. :D

Fixes while(!Serial); not waiting on the teensy 3.0/3.1
Fixes a type pun in usb_midi.c
Fixed uint8_t -> uint16_t for the control status in usb_serial. A uint8_t is too small to hold the information.

Misc:
Speed up compile.

Advanced features:

Ability to completely disable the USB code. Handy if you don't need to use the USB interface.
Ability to easily reconfigure the provided device types. You can mix and match more than what is normally available.
Ability to easily add new device types.

The advance features require that you know what you are doing, and are comfortable with using the command line.
You will need to download an additional tool and install it in your sketch directory.
The download is here and allows you to compile sketches from the command line and tweak all sorts of settings.
The advanced stuff has been tested to work on Linux, and should work on Mac. Windows will work after I figure out how to make it happy with Cygwin :rolleyes:

More details on the advanced stuff soon... Like, some good examples. :cool:
 
Last edited:
Why did you add a 10 ms delay in the Serial bool operator? If there's a race condition or other problem, isn't there some better way to fix it? I would like to address this issue (which I don't fully understand yet), but adding a 10 ms delay seems pretty drastic.

I also don't understand the change on usb_configuration from 8 to 16 bits. Sure, the host sends it in a 16 bit wValue field, but the only legal values are 0 and 1. Even if the USB stack were someday extended to support a multi-configuration device, the USB standard doesn't support more than 255 configurations, because "bNumConfigurations" in the device descriptor and "bConfigurationValue" in the configuration descriptor are only 8 bits. Details can be found in the USB 2.0 spec on pages 263 and 265. On page 265, bConfigurationValue is described with "Value to use as an argument to the SetConfiguration() request to select this configuration". Even though the SetConfiguration request sends bConfigurationValue in the 16 bit wValue field, it can never have more than 8 bits because it's only allocated 8 bits by the USB standard.
 
is xxxajk sanctioned for this?
A fork of the core is disconcerting from a config. management viewpoint.
 
Yes, it's fine. In fact, that's pretty much how github works.

I do want to incorporate improvements back into the official version. But I have some reservations about some of these changes....
 
Why did you add a 10 ms delay in the Serial bool operator? If there's a race condition or other problem, isn't there some better way to fix it? I would like to address this issue (which I don't fully understand yet), but adding a 10 ms delay seems pretty drastic.
It isn't a race condition as much as it seems to be more of waiting for the PC to be ready. Yes, it seems drastic, but it seems to fix it for me, and it does fix it on the USB interfaced Arduinos I have that have this exact same issue. I believe if it is a race, the problem is because the change in the value is not atomic. For whatever reason, the delay does work. If Yield() actually does something useful (does it?) that would be a good alternative on the Teensy.

I also don't understand the change on usb_configuration from 8 to 16 bits. Sure, the host sends it in a 16 bit wValue field, but the only legal values are 0 and 1. Even if the USB stack were someday extended to support a multi-configuration device, the USB standard doesn't support more than 255 configurations, because "bNumConfigurations" in the device descriptor and "bConfigurationValue" in the configuration descriptor are only 8 bits. Details can be found in the USB 2.0 spec on pages 263 and 265. On page 265, bConfigurationValue is described with "Value to use as an argument to the SetConfiguration() request to select this configuration". Even though the SetConfiguration request sends bConfigurationValue in the 16 bit wValue field, it can never have more than 8 bits because it's only allocated 8 bits by the USB standard.
It avoids type punning and overloading/casting that is unclean code-wise.
Is the loss of this one byte in RAM a real problem, or is there a better way to avoid the punning/confusing code and standards mess?
Why are there 16 bits and not 8 anyway if 8 are not used? What is actually sent on the wire? If it is 8bits, then I can easily change that back. :confused:

How about the macros for USB? You like?
 
I haven't reviewed all this yet, and I probably won't for a while. At the moment, I need to go visit our PCB assembly vendor regarding multiple mundane but critically important things that keep PJRC running.

I'm also working pretty heavily on the audio library. Until it's at a 1.0 release (ideally within 2 weeks), which at this point is probably just the fader object, a few missing functions, bug fixes and a LOT of documentation, I'm probably not going to have a lot of time to spend on this.

On the bool operator, can you explain the issue a bit? Maybe post details on a test case? I have occasionally seen weirdness there, so I'm sure you're on to something here. If a 10 ms delay is the only way to fix it, then that will be the fix. But my gut feeling is a much better fix ought to be possible once the problem is better understood.
 
As far as the bool operator, I'll have to dig in and look at the asm. One way to avoid a race (if it indeed is one) would be to save the result to a variable while wrapped in a cli()/sei(), and then return it.

One of the oddest things I have ran into with booleans in C++ is that you can't actually compare two of them, which leads to silly code like the following:
Code:
boolean OK = UHS_USB_BulkOnly[B]->LUNIsGood(LUN);
// C++ can't compare boolean to a boolean -- stupidity++;
if((last_ready[B][LUN] && !OK) || (!last_ready[B][LUN] && OK)) { // state changed
...
Yep, you can't just do
Code:
if(last_ready[B][LUN] != OK)

So I really doubt it is atomic either.
 
The delay (if you read the reasons) is that the interface is half configured sometimes.
(again as on Github) Here is the comment:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/cores/arduino/CDC.cpp#L225-L238

It is a race that you can not get control over due to the very nature of USB. It takes a few extra ms for the USB packets to fly back and forth to actually finish the setup.
In short, the PC isn't ready yet... it has not gotten or processed these packets yet. They may be sitting in a queue on a busy host.
At the same time, the USB stack on the teensy (as far as it is concerned) is ready.

Basically you got something half open, and the PC is going to reject the packets, and the teensy will throw out the packets if too many are sent at the start.
It is not necessarily the fault of the PC, or the particular OS, or the teensy stack. It is just simply cumulative latency of getting told the setup is done, getting the reply back to the PC, and setting the fact that the interface is ready on the PC. Some OS may be even loading a driver at this point (windoiws??), and others (llinux, mac, etc) take a touch of time to create the inode in usbfs...
 
As far as the uint16_t business... I looked it over and you are right... and I also have a suggestion as well.
It may be better code-size-wise and execution-speed-wise to add the actual byte accessed within the struct definition as an additional union, instead of shifting the bytes manually.
 
As far as the uint16_t business... I looked it over and you are right... and I also have a suggestion as well.
It may be better code-size-wise and execution-speed-wise to add the actual byte accessed within the struct definition as an additional union, instead of shifting the bytes manually.
Or you may find the compiler actually optimizing the load/shift sequences to be an appropriate load byte/16-bit quantity.

Of course if you play fast and loose with things like this, you may get burned in a future project where there are different assumptions (for example, if you get a Navispark which uses a sparc v8 processor, which is only big endian, while the standard AVR/ARM Arduino type environment is little endian). As the internet sage Henry Spencer used to have as his signature line in the 1990's: 'Do not lie to the compiler, for it will get its revenge'. Seriously, as the compiler gets smarter, we find code that does non-standard games with pointers, will not work.
 
ntohs() and htons() takes care of this sort of thing when you know you are dealing with network byte ordered stuff--
USB is a network, and uses big endian, aka network host ordered data.
 
Here is exactly the problematic line... https://github.com/xxxajk/cores/blob/130f11306c732b3996d0c32023c57fed7d6ad79b/teensy3/usb_dev.c#L169
So which byte do we need here? the first or the second?

Please take a look at page 257 of the USB 2.0 specification (the 285th page in the PDF file), section 9.4.7.

The specific text from the USB 2.0 spec is:

The lower byte of the wValue field specifies the desired configuration. This configuration value must be
zero or match a configuration value from a configuration descriptor. If the configuration value is zero, the
device is placed in its Address state. The upper byte of the wValue field is reserved.


It looks like the second is grabbed on ARM, so, is that a bug or is that the part we want?

I am still struggling to understand why you are calling this a bug?
 
This thread has become a confusing mix of discussion regarding 2 patches I have reviewed, more patches I haven't reviewed yet (and may not for several days or perhaps not until after a 1.0 audio library release), some other code I don't recognize that appears to be from the USB Host Shield library, and finer points of compiler semantics and maybe other stuff too?

It is indeed rare that I ask anyone to keep any conversation on-topic or posted in the appropriate forum. But in the case of patches to the Teensy 3.X core library, and specifically whether I will merge them, the sprawling and divergent mix of topics being discussed in this single thread is not improving the likelihood I'll accept these patches.

At least for these 2 I have reviewed:

1: I do not believe the use of uint8_t for usb_configuration is a bug.

2: Use of delay(10) in the Serial bool operator feels like a very ugly kludge. I'm not denying there's a problem that needs to be addressed. I just don't like this solution. That doesn't mean it's rejected yet, but before I accept this, I really want to know more about the problem it solves. So far, not even a specific test case has been described.
 
Yes, please cherry-pick. That is the entire idea. :)
As far as the uint8_t business, I fixed it farther down in the patch set.
The delay, I'll let you decide, however it does fix things for me, and eliminates the need for me to do a delay in setup every time.

Now for some good news:
What I have been able to do with the other goodies is make my own bulk drivers, my own descriptors, etc.
This is something I can't do from the IDE (yet, because the whole Arduino IDE I believe was rushed, and not your fault).
I do however wonder if you can do an include like the Makefile does, and include the sketch dir. On make it is as simple as "-I ./".

Now for a question for you Paul... Why does only a 64byte endpoint work?
I tried increasing this to various values, and the teensy 3 rejects packets. Is there some setting preventing it from working? The buffers are the correct size, there is no corruption, etc.
My only guess is that perhaps deeper in the core it is only setting 8 of the available 10 bits on the USB-FS. Is that the reason or is there something more to it?
I'm working on (amongst other things) a bulk driver, so you can 'activate' it in the core. Sure I could use 64 bytes, but 512 is way better in terms of transaction latency.
 
It is indeed rare that I ask anyone to keep any conversation on-topic or posted in the appropriate forum. But in the case of patches to the Teensy 3.X core library, and specifically whether I will merge them, the sprawling and divergent mix of topics being discussed in this single thread is not improving the likelihood I'll accept these patches.

Paul, regarding Teensyduino patches, can you start perhaps a stickied thread where people can post patches or bugfixes? Right now they end up loose in the forum or lost in PM/email.

EDIT: just a thought, if there is concern that it will degrade into a jumble of all manner of topics, perhaps ask that people post links to threads instead of dumping content itself...
 
Last edited:
Status
Not open for further replies.
Back
Top