Skip to content

Fix endpoint checking; Clean up safe mode printing #4754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 14, 2021

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented May 12, 2021

Fixes #4746.
Fixes #4752 (STM32 HID support).

  • Endpoint count checking was off by one.
  • CIRCUITPY_USB_{HID,MIDI,MSC}_ENABLED_DEFAULT default values were not set conditionally.
  • Clean up print_safe_mode_message() to simplify logic, reduce size, and divide up safe-mode reasons more accurately.
  • Redo logic of printing startup messages in run_code_py() to not print the safe-mode message twice, clean up newlines, and be more consistent about whitespace.
  • Shortened and rephrased some messages slightly.
    EDIT:
  • Count in and out endpoints and check against provided limits. ESP32-S2 allows only 5 IN endpoints, so this enforces that limit
  • Turn on MIDI and HID on some low-endpoint boards, but have them disabled. The user could disable something else (like the REPL) to add them.
  • Add more information to documentation.

It appears to me that a translate() message with a \n in it is actually printed as a \r\n by serial_write_compressed(). I'm not sure if that's done during printing or during compression. But a bare serial_write("\n") does not print a \r. So I added \r as necessary in a few places.

@dhalbert dhalbert requested review from tannewt and jepler May 12, 2021 20:01
jepler
jepler previously approved these changes May 13, 2021
Copy link

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK, no testing performed.

@Neradoc
Copy link

Neradoc commented May 13, 2021

It did not fix #4746 in my tests, both cases with issues now do a reset loop.

@dhalbert
Copy link
Collaborator Author

It did not fix #4746 in my tests, both cases with issues now do a reset loop.

Thanks, because of debugging issues I was testing with an STM board, but will try with an ESP32-S2.

@dhalbert dhalbert dismissed jepler’s stale review May 13, 2021 04:10

still broken

@dhalbert
Copy link
Collaborator Author

dhalbert commented May 13, 2021

@Neradoc It turns out the ESP32-S2 endpoint situation is idiosyncratic. From the datasheet:

  • Endpoint number 0 always present (bi-directional, consisting of EP0 IN and EP0 OUT)
  • Six additional endpoints (endpoint numbers 1 to 6), configurable as IN or OUT
  • Maximum of five IN endpoints concurrently active at any time (including EP0 IN)

All the other chips have symmetric pairs of IN/OUT endpoints, but the ESP32-S2 does not. I am confused why we can support HID at all, because there don't appear to be enough singular endpoints. It's late and I will look in the morning. In any case, I will need to restrict the choices. It will be possible to add more HID devices, but I think the CDC data channel is not possible. (EDIT: misunderstanding on my part due to the way the datasheet was written. I looked at the code.)

So there are six additional endpoint pairs, but there can be only up to 5 IN endpoints among EP0 and the six. We need EP0, and then we need four more: one IN for MSC, one IN for HID, two IN per CDC (control and data). So there are not enough for the secondary CDC unless MSC or HID is turned off. There could be MIDI. I'll have to decide how dynamic I can make this, There are static allocations in TinyUSB for each device.

@dhalbert
Copy link
Collaborator Author

Note edit in previous comment.

@tannewt tannewt added this to the 7.0.0 milestone May 13, 2021
@tannewt tannewt added the usb label May 13, 2021
tannewt
tannewt previously approved these changes May 13, 2021
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. It might be good to try an update the translations so that the ones aren't lost about button presses.

@dhalbert dhalbert dismissed tannewt’s stale review May 13, 2021 21:44

I'll be making more changes; dismissing just so it won't be merged.

@dhalbert dhalbert force-pushed the fix-endpoint-checking branch from 5f639f3 to fa6c06f Compare May 14, 2021 01:59
@dhalbert
Copy link
Collaborator Author

Added:

  • Count in and out endpoints and check against provided limits. ESP32-S2 allows only 5 IN endpoints, so this enforces that limit.
  • Turn on MIDI and HID on some low-endpoint boards, but have them disabled. The user could disable something else (like the REPL) to add them.
  • Add more information to documentation.

make translate seems to have fixed up some of the messages that would otherwise have been lost due to rewriting. I will check after merging and address missing ones from weblate if I can.

@dhalbert dhalbert requested a review from tannewt May 14, 2021 03:22
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks!

@tannewt tannewt merged commit 689ec86 into adafruit:main May 14, 2021
@dhalbert dhalbert deleted the fix-endpoint-checking branch May 14, 2021 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STM32(F4) USB HID support changing USB descriptors on ESP32S2 crashes
4 participants