Skip to content

ESP32S2 USB Persistence and Stability#446

Closed
me-no-dev wants to merge 1 commit into
hathach:masterfrom
me-no-dev:esp32-s2-persistence
Closed

ESP32S2 USB Persistence and Stability#446
me-no-dev wants to merge 1 commit into
hathach:masterfrom
me-no-dev:esp32-s2-persistence

Conversation

@me-no-dev
Copy link
Copy Markdown
Collaborator

This change implements proper handling of the USB peripheral, when the ESP32S2 is booting with USB Persistence enabled.
Also add some small fixes here and there to contribute to more stable USB.

There is one new function added to usbd. I did not find a better way to do what it does otherwice. Ideas are welcome

This change implements proper handling of the USB peripheral, when the ESP32S2 is booting with USB Persistence enabled.
Also add some small fixes here and there to contribute to more stable USB.

There is one new function added to usbd. I did not find a better way to do what it does otherwice. Ideas are welcome
@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

Thanks for the PR, could you explain what is booting with USB Persistence and why it may be needed ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

On ESP32S2 we can reboot into download mode, flash the device and reboot back into the new firmware without re-enumerating. Basically the OS is not aware that the device in reality rebooted into different mode and then back.
It works exactly the same as if a normal USB-UART chip is used.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

To be clear, the code here does not add any functionality to TinyUSB, it just detects if the reboot was persistent and handles the interfaces properly (events that come when the peripheral is reset will not be coming). Unless such reboot is triggered, the code will behave exactly as before, with the exception of the few fixes added to help in some cases.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

On ESP32S2 we can reboot into download mode, flash the device and reboot back into the new firmware without re-enumerating. Basically the OS is not aware that the device in reality rebooted into different mode and then back.
It works exactly the same as if a normal USB-UART chip is used.

Interesting, I don't know it is possible, look like esp32s2 cpu can reboot without resetting the USB peripheral, which is very neat.The synopsys controller must be NAKing all request from host while in DFU mode. Do you have any example that I could run on saola board, I would like to capture the usb bus in various scenarios.

PS: I still have a question, when in download mode, does the boot code of esp32s2 use USB or just leave it as it is when rebooting ??

USB0.gotgint = ~0; //clear OTG ints
USB0.gintsts = ~0; //clear pending ints
enum_done_processing();
usbd_force_reconfig(rhport, 1);
Copy link
Copy Markdown
Owner

@hathach hathach Jun 29, 2020

Choose a reason for hiding this comment

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

DCD is lowest level, it is not its responsibility to tell usbd to go into configured mode. In other way, this is too hacking and wouldn't got approved.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, I accept ideas :) You know your software better. Given the position of the call, do you have a better idea of where to place it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

maybe I can call it in our code after tusb_init(). Will that be OK?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

uhmmm.. :) I kinda do, but at the current state it will not be easy.
First, how it works:
our ROM bootloader contains CDC+DFU driver on it's own and the ability to not reset the USB if told so before rebooting.
When we match the endpoints of both TinyUSB and ROM CDC, we can have the USB communicating properly with both. The time when nothing is responding is between second stage bootloader and tinyusb init.

The esp32s2 branch of https://github.com/espressif/arduino-esp32/tree/esp32s2 already contains a higher level of implementation (that does persistent reboot), but tinyusb and the IDF layer is precompiled (remember the earlier conversation on the other topic?). The precompiled IDF layer and TinyUSB include changes that are not upstream, therefore no way for you to get them unless I send you the files directly. You can give it a shot with Arduino if you want to see it working, or let me know how you want to proceed?

Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thanks for the pr, the idea of persistent USB is neat, and probably only possible with esp32s2. I am not sure if it is applicable to STM with the same IP. However, the proposed solution here is too hacky, and wouldn't get approved in anyway. We will need a better alternative, I will close this PR shortly, you could file an issue for discussing on how we could implement it in the alternative. As the rule of thumb, the API flow goes from high to low. DCD can only report its status but cannot instruct how usbd operate such as go into configured mode.

Regarding other minor changes of the DCD, it should be kept as minimal as possible, since I have a plan to merge esp32s2 and stm32 dcd into a single synopsys driver file. #382

desc_edpt->bmAttributes.xfer << USB_D_EPTYPE1_S |
(desc_edpt->bmAttributes.xfer != TUSB_XFER_ISOCHRONOUS ? (1 << USB_DI_SETD0PID1_S) : 0) |
desc_edpt->wMaxPacketSize.size << 0;
uint8_t selected_fifo = 0;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you explain which problem the selected_fifo and dcd_allocated_fifos are used to solve ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ESP32S2 USB peripheral have 7 endpoints, but only 5 FIFOs. The code makes it possible to have devices using EP5 or 6 and decouples the FIFO number from the endpoint number.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The problem is you won't use EP5 and EP6 if EP1-EP4 are not all taken, which will effectively make EP5,6 not usable as IN endpoint. So it will be just better to limit the number of endpoints to number of FIFO.

Comment thread src/device/usbd.c
dcd_event_handler(&event, in_isr);
}

// Can be called by the dcd driver when rebooting wirh persistent USB
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

DCD cannot be the one to call this function, since USBD is the higher API level

ESP_EARLY_LOGV(TAG, "dcd_int_handler - suspended");
USB0.gintsts = USB_USBSUSP_M;
//dcd_event_bus_signal(0, DCD_EVENT_SUSPEND, true);
dcd_event_bus_signal(0, DCD_EVENT_UNPLUGGED, true);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

suspend is not unplugged, this should be DCD_EVENT_SUSPEND

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it is in our case.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it is a side effect, try to put your PC into sleep to see if the event comes up ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

I am not sure who you need approval from for ESP32S2 driver changes? Given that it's used on ESP devices and I am an Espressif engineer, does it not make sense to accept changes? The hacky way is needed exactly because no other MCU supports it, therefore your stack also does not support it. This is the lightest possible change to make that feature work. The feature is important and users are looking forward to it. This PR refusal just means that those files will still be separate in ESP-IDF. Given that ESP-IDF and Arduino provide TinyUSB functionality to ESP32S2 users and that they will get that changed version, how does it not make sense to have it upstream?

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

uhmmm.. :) I kinda do, but at the current state it will not be easy.
First, how it works:
our ROM bootloader contains CDC+DFU driver on it's own and the ability to not reset the USB if told so before rebooting.
When we match the endpoints of both TinyUSB and ROM CDC, we can have the USB communicating properly with both. The time when nothing is responding is between second stage bootloader and tinyusb init.

I think we have a bit of issue here, firstly application configuration descriptor must match the ROM bootloader one. Although the CDC is easy, the DFU can be trouble since in application the DFU interface is only Runtime (for trigger into download mode), while ROM bootloader is actual DFU Mode. You probably don't see the issue when uploading via CP210x, though doing that via DFU interface can frustrate the host. That is not mention the new application can has different set of composite driver e.g adding midi/msc driver. There is too many issues with this persistent USB. I think the advantage of this is probably not worth the trouble, PC software e.g Arduino is used to detect and reconnect to newly attached serial port without any issues.

The esp32s2 branch of https://github.com/espressif/arduino-esp32/tree/esp32s2 already contains a higher level of implementation (that does persistent reboot), but tinyusb and the IDF layer is precompiled (remember the earlier conversation on the other topic?). The precompiled IDF layer and TinyUSB include changes that are not upstream, therefore no way for you to get them unless I send you the files directly. You can give it a shot with Arduino if you want to see it working, or let me know how you want to proceed?

I don't see why precompiled tinyusb can cause any issue with this, could you explain the issue you are having with it ?

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

I am not sure who you need approval from for ESP32S2 driver changes?

Just to clear, I am the one who disapproves this PR. As I said in the review, this proposed solution is not good enough to go into upstream.

@hathach hathach closed this Jun 29, 2020
@me-no-dev
Copy link
Copy Markdown
Collaborator Author

The configuration descriptor has no problem to match. They are in fact the same. First is added CDC and then DFU. Persistence is available when ONLY CDC and DFU are enabled. Any other configuration will disable Persistence.
Application/Device descriptors and such are out of the scope of TinyUSB, we have the option to select endpoint numbers and configure interfaces as necessary. Both CDC and DFU are working fine with Persistence. We can upload/download/monitor etc.

@me-no-dev me-no-dev deleted the esp32-s2-persistence branch June 29, 2020 18:08
@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

The configuration descriptor has no problem to match. They are in fact the same. First is added CDC and then DFU. Persistence is available when ONLY CDC and DFU are enabled. Any other configuration will disable Persistence.
Application/Device descriptors and such are out of the scope of TinyUSB, we have the option to select endpoint numbers and configure interfaces as necessary. Both CDC and DFU are working fine with Persistence. We can upload/download/monitor etc.

The DFU interface's bInterfaceProtocol in application mode is Runtime (0x01) while the one in bootloader mode is DFU mode (0x02). Maybe the PC host is more forgiving on that. However it puts such as limit on application, why would you want to implement it ? I mean what is the benefit of that ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

There are in fact a few ways to boot into download mode and one of them is specifically DFU with persistence. Sorry I forgot to mention that. running dfu-util -R -U file.dfu works. There are no modifications done to dfu-util or anything else

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

I think that DFU-UTIL itself queries the device state and senses the change of the mode. If it actually expects the device to reenumerate, it will not work, because the descriptor will still say that it's in runtime mode.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

Here is an output in the console. You can see that the device is first detected as Run-time device DFU version 0101 and then as DFU mode device DFU version 0110 when mode was switched.

dfu-util -R -U dump.dfu
dfu-util 0.9

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2016 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Opening DFU capable USB device...
ID 303a:0002
Run-time device DFU version 0101
Claiming USB DFU Runtime Interface...
Determining device status: Device does not implement get_status, assuming appIDLE
Device really in Runtime Mode, send DFU detach request...
dfu-util: error detaching
Resetting USB...
Opening DFU USB Device...
Claiming USB DFU Interface...
Setting Alternate Setting #0 ...
Determining device status: state = dfuIDLE, status = 0
dfuIDLE, continuing
DFU mode device DFU version 0110
Device returned transfer size 64
Copying data from DFU device to PC
Upload	[=========================] 100%      4194304 bytes
Upload done.
dfu-util: can't detach

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

regardless of what dfu-util implementation, here is DFU specs, runtime for application, dfu mode for bootloader. And it should re-enumerate.
image

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

given that the descriptors are the same and what you post above, is it not OK the ROM to return the proper DFU descriptor and things to continue to work? It does not say that you need to reenumerate, only that must provide proper descriptor.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 29, 2020

USB VID/PID for bootloader and application should also be different as well. There are too many problems, could you tell me the benefit of this again ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

I understand that you might be worried that issues might come if this feature is enabled, but I would like to point out that it first must be specifically supported by the upper layer and that the control of when it's on is strict to as I said DFU and CDC. DFU is in this case only legacy and not expected to be used. I will be testing different configurations and DFU's persistence is not set in stone. The feature has to do with only CDC and the possibility to use ESP32S2 devices with our tools transparently.
Without that feature specifically enabled with a flag to the bootloader, tinyusb works as usual. Some of the changes included had to do with issues I came along in both normal and persistent mode. By default the driver was matching FIFO numbers to Endpoint numbers, assuming that the number of fifos equals the number of endpoints. After much headbanging and discussion with @igrr, the proposed solution was something close to what was included. I also ended up in cases where because only bits from masks were set, but the masks were not cleared, some endpoints were broken or writing to the wrong fifo. Just like you, I was surprised that unplugging the cable resulted in SUSPEDED, but that and EARLY_SUSPEND were the only interrupts triggered. Missing endpoint interrupt was also discovered, that if not cleared, would lock the interface and only full reset will fix it.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

USB VID/PID for bootloader and application should also be different as well.

I guess you mean for DFU? If so, that is not an issue. If DFU requests the reboot, persistence can be turned off and device will reenumerate with different PID. For CDC, there is no requirement.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

Re previous issue: Because ESP-IDF is quite large and it's structure, it's impossible to be included in Arduino as source. Instead ESP-IDF and it's components (including TinyUSB) are precompiled and only linked against with Arduino later. That means that in order for Arduino to have support for all drivers that TinyUSB have, they all have to be built with the libs. But then if all are built, all are loaded, which takes memory and space.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

I got intrigued by your DFU notes and investigated further. I found an issue in my code caused by wrong handling of the control packets and rebooting slightly early, due to MacOS differences. The issue is now resolved with the example changes provided in #450

Once that got fixed, dfu-util actually requested the bootloader to reenumerate, which follows the rules you outlined. Thanks!

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 30, 2020

USB VID/PID for bootloader and application should also be different as well.

I guess you mean for DFU? If so, that is not an issue. If DFU requests the reboot, persistence can be turned off and device will reenumerate with different PID. For CDC, there is no requirement.

No, I mean what is the benefit of persistent USB and why you want to implement it, since it impose restriction on application USB and having other potential issues as well.

Without that feature specifically enabled with a flag to the bootloader, tinyusb works as usual. Some of the changes included had to do with issues I came along in both normal and persistent mode. By default the driver was matching FIFO numbers to Endpoint numbers, assuming that the number of fifos equals the number of endpoints. After much headbanging and discussion with @igrr, the proposed solution was something close to what was included. I also ended up in cases where because only bits from masks were set, but the masks were not cleared, some endpoints were broken or writing to the wrong fifo

I don't see the benefit of having FIFO with EP5, EP6. USB class driver almost always have EP IN >= EP OUT, therefore I don't see the practical usage of this.

Just like you, I was surprised that unplugging the cable resulted in SUSPEDED, but that and EARLY_SUSPEND were the only interrupts triggered.

I am not surprised that unplug device cause suspend at all, this is the side effect due to how USB specs define the suspend condition. However, that is not how you detect unplug event, to sum up.

  • Unplug -> cause suspend
  • PC enter sleep mode --> also cause bus suspend

Therefore you cannot use bus suspend to detect unplug.

Missing endpoint interrupt was also discovered, that if not cleared, would lock the interface and only full reset will fix it.

Can you file an issue and PR separated for this, this issue is not in my radar and I haven't encountered it yet.

Re previous issue: Because ESP-IDF is quite large and it's structure, it's impossible to be included in Arduino as source. Instead ESP-IDF and it's components (including TinyUSB) are precompiled and only linked against with Arduino later. That means that in order for Arduino to have support for all drivers that TinyUSB have, they all have to be built with the libs. But then if all are built, all are loaded, which takes memory and space.

I see, it is purely for the space reason. How much spaces you are saving with this, could you give me your number.I have ported TinyUSB to both SAMD and nRF5x, the footprint of the built-in driver is small enough to just enable them all on those smaller MCUs, it should be virtually no issue with esp32s2.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

I don't see the benefit of having FIFO with EP5, EP6. USB class driver almost always have EP IN >= EP OUT, therefore I don't see the practical usage of this.

These are endpoint INs, not OUTs, so you have more OUT than IN, not what you expected. EP OUTs use different fifo.

No, I mean what is the benefit of persistent USB and why you want to implement it, since it impose restriction on application USB and having other potential issues as well.

If by restriction, you mean that it's active only when particular devices are enabled, where is the issue? It works only on CDC, which is not a problem. With every other interface it restarts as usual and reenumerates.

I am not surprised that unplug device cause suspend at all, this is the side effect due to how USB specs define the suspend condition. However, that is not how you detect unplug event, to sum up.

How would you go by detecting unplug in this case? SUSPEND is the only event that comes. If not acted upon, the app does not get the tud_umount callback and is not aware that something happened. I also tried the SUSPEND event (that is why the call is commented there and left to poke me when I am going through the code), but that did not work as expected on RESUME.

Can you file an issue and PR separated for this, this issue is not in my radar and I haven't encountered it yet.

I got it a couple of times. Can not recall the exact steps to reproduce now, so can not file a bug report. It's not easy to trigger.

I see, it is purely for the space reason. How much spaces you are saving with this, could you give me your number.I have ported TinyUSB to both SAMD and nRF5x, the footprint of the built-in driver is small enough to just enable them all on those smaller MCUs, it should be virtually no issue with esp32s2.

You'll be surprised how particular people are about memory and flash usage, regardless of the vast amounts available compared to most other MCUs.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 30, 2020

I have asked this question multiple times, but you didn't provide the answer. Let's be clear that if the reason/benefit does not make sense to me, it won't get approved into the upstream even with a better solution than this current hack one.

What is the benefit of persistent USB, what problems does it solve ?

For all other things, please open an separated issue and we can discuss specific on those afterwards

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

What is the benefit of persistent USB, what problems does it solve ?

It makes the native USB+CDC work with the same tools that we use for all other ESP chips. That means flashing, monitoring and all. This is the first native USB chip for espressif, so all tools are expecting to be connected to USB-UART and be able to toggle RTS and DTR without the device that they are connected to to disappear and possibly appear with different pid/vid. ESP chips use 2 lines to reset into download (RST and one bootstrap pin), so toggling the lines in particular way is needed.
Without persistence enabled, one has to manually enter download mode in order to flash the chip with the new firmware.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

me-no-dev commented Jun 30, 2020

Use case: Arduino user with open monitor or ESP-IDF user in the same case

Example workflow with Persistence:

  • click upload in ArduinoIDE or Ctrl+T Ctrl+A in IDF monitor
  • monitor will be paused, new firmware loaded and device rebooted
  • monitor will auto-connect back

Example workflow without persistence:

  • close monitor first
  • toggle buttons to put device into download mode
  • click upload/idf.py flash (if VID/PID are the same)
  • reconnect manually the monitor after flashing is done

As you see, without persistence, everything gets much more complicated and requires user to act on each step

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 30, 2020

What is the benefit of persistent USB, what problems does it solve ?

It makes the native USB+CDC work with the same tools that we use for all other ESP chips. That means flashing, monitoring and all. This is the first native USB chip for espressif, so all tools are expecting to be connected to USB-UART and be able to toggle RTS and DTR without the device that they are connected to to disappear and possibly appear with different pid/vid. ESP chips use 2 lines to reset into download (RST and one bootstrap pin), so toggling the lines in particular way is needed.
Without persistence enabled, one has to manually enter download mode in order to flash the chip with the new firmware.

Thanks for explanation, finally I could understand the reason for persistent mode. However, it is still not clear to me how the persistent mode could help with the flashing and reboot to download mode. Could you break out the steps in reboot into download mode what is the difference between persistent and normal mode ?

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jun 30, 2020

Use case: Arduino user with open monitor or ESP-IDF user in the same case

Example workflow with Persistence:

  • click upload in ArduinoIDE or Ctrl+T Ctrl+A in IDF monitor
  • monitor will be paused, new firmware loaded and device rebooted
  • monitor will auto-connect back

Example workflow without persistence:

  • close monitor first
  • toggle buttons to put device into download mode
  • click upload/idf.py flash (if VID/PID are the same)
  • reconnect manually the monitor after flashing is done

As you see, without persistence, everything gets much more complicated and requires user to act on each step

Actually I don't really know how the persistent mode could help. For Arduino you don't really need to close monitor before uploading, just click upload, the IDE will temporarily gray-out the Serial ,wait for new port to be available and perform update. We have done that using touch1200 for both nRF and SAMD with great success.

Just to be clear the board here is uploading using native USB without the help of CP210x/FTDI bridge right ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

  • The board is connected only through the native USB port
  • Unfortunately our firmware tool acts a bit differently than what the regular Arduino Serial upload is and that will cause extra reboot that will not be expected. If PID/VID/Serial mismatch with what is in the ROM, the device will enumerate at a new port that will then be unknown. With persistence, the device stays the same throughout the process, so port location is not changed.
  • Arduino aside, none of the tools that we have, support such functionality

Here is more or less how a persistent serial flash is performed:

  • esptool.py connects to the CDC port (rts&dtr are high)

  • esptool.py then send a series of 4 Line State changes, toggling rts&dtr in order to get an ESP connected through regular UART booted into download mode

  • the application detects the changes and then:

    1. Sets registers to disable IO and USB reset
    2. Sets the USBDC_PERSIST_ENA bit in the date register of USB_WRAP
    3. Disables Timer Group 1 (else download will be interrupted)
    4. Sets register to force reboot into download
    5. Resets the CPU
  • Now the device is in download mode

    1. ROM CDC reads the USBDC_PERSIST_ENA bit
    2. If set, will not reset USB and do more or less what is in this PR as persistence changes
    3. Flashing will now start
    4. ROM will set again USBDC_PERSIST_ENA bit and disable USB reset
    5. Reboot into the new firmware
  • Boot goes through the bootloaders and starts the firmware without touching the USB

  • Firmware starts TinyUSB and the cycle can be repeated

If there are different devices other than CDC and DFU loaded, USBDC_PERSIST_ENA bit will be cleared, so that TinyUSB will restart the bus and reenumerate the devices. Following attempt to flash over CDC will not use persistent mode.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

We have done that using touch1200 for both nRF and SAMD with great success.

This will/can not work on ESP32S2. That is because it needs both lines in specific order. I have not debugged how touch1200 looks, but I am pretty sure that all it does is connect and disconnect (slowly) toggling DTR and causing reboot. do nRF and SAMD always go through the bootloader that can flash the device? Because that is not the case for ESPs. I know AVR works like that, the STM I used today as well, but not ESPs.

@tannewt
Copy link
Copy Markdown
Collaborator

tannewt commented Jun 30, 2020

@me-no-dev Do you have a PR that splits out the USB fixes from the persistence? That'd be helpful to not block the fixes.

I'd also encourage you to make PRs instead of an issue if you have example code that fixes an issue. That way it's much easier to see the change and the context.

Regarding persistence, I don't think that TinyUSB is the best place to fix it. Reconnecting to a device should be up to the host, not the device. However, if we can make it appear as the same device that would make things easier. What platforms does it change on? Would it help to have the same serial number or does USB ID changes prevent it? I think HID can influence the name as well on Mac at least.

In CircuitPython, we've implemented the 1200 baudrate trick that arduino uses. It is implemented here: https://github.com/adafruit/circuitpython/blob/main/supervisor/shared/usb/usb.c#L109-L125 The reboot into the bootloader is done by setting an in-memory flag that the bootloader checks at startup.

For ESP32-S2 Arduino have you consider not using the ROM bootloader except in recovery? Our plan is to use a UF2 bootloader in the factory partition because mass storage bootloading is much easier for users because it requires no extra tools. (It can also still present CDC and DFU flashing as well.) Our UF2 bootloader is here: https://github.com/adafruit/uf2-esp32s

Note, I'm available on the Adafruit Discord and we've added a new #tinyusb channel there. https://discord.gg/ZyF7653

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

@tannewt

Do you have a PR that splits out the USB fixes from the persistence? That'd be helpful to not block the fixes.

I'll open new one, no problem.

I'd also encourage you to make PRs instead of an issue if you have example code that fixes an issue.

I was really confused by the fact that there was no issue already existing for it, so I thought that there is still a chance that I am missing something or that it was known. Having the second platform to confirm, helped.

Regarding persistence, I don't think that TinyUSB is the best place to fix it. Reconnecting to a device should be up to the host, not the device.

The host is not requesting reset of the device in this case. We in effect just emulate a regular USB-UART chip. They do not reset/enumerate when you are flashing a device. The code in TinyUSB is not a fix, but rather a way to handle the situation. On Mac at least the device will reenumerate with new port if VID or PID or Serial is changed. ROM CDC have Serial 0, which is not very convenient. Changing the name I think also caused new port on my Mac.

In CircuitPython, we've implemented the 1200 baudrate trick that arduino uses.

Even if this method is used, it will still not help if the device gets a new port. And this is an ArduinoIDE solution. Will not otherwise work in IDF.

For ESP32-S2 Arduino have you consider not using the ROM bootloader except in recovery? Our plan is to use a UF2 bootloader in the factory partition because mass storage bootloading is much easier for users because it requires no extra tools. (It can also still present CDC and DFU flashing as well.)

Of course all these options will be available :) Persistence could only be available if just CDC or CDC and DFU are enabled. Any other interface would turn off Persistence and trigger a reset. It makes a lot of sense to reset everything if you are updating over MSC or Vendor, but not so much with UART :)
Again, Persistence is CDC-Only thing. When DFU is also added, they both match the same descriptor as in the ROM, thus both work fine (DFU actually resets and re-enumerates).

Note, I'm available on the Adafruit Discord and we've added a new #tinyusb channel there. https://discord.gg/ZyF7653

Thanks!

p.s. As I think about it, the only time when the device is not able to respond to CDC commands, is when it's booting into the firmware, but the monitor has not yet connected, so really nothing wrong is happening on the bus. And even if the host sends a command, the specs allow the command to not be responded to if CRC failed. The worst in that case is the host to reset the device and go through full enumeration, which will be handled again just fine by TinyUSB.

@tannewt
Copy link
Copy Markdown
Collaborator

tannewt commented Jun 30, 2020

I'll open new one, no problem.

Thanks!

Regarding persistence, I don't think that TinyUSB is the best place to fix it. Reconnecting to a device should be up to the host, not the device.

The host is not requesting reset of the device in this case. We in effect just emulate a regular USB-UART chip. They do not reset/enumerate when you are flashing a device. The code in TinyUSB is not a fix, but rather a way to handle the situation. On Mac at least the device will reenumerate with new port if VID or PID or Serial is changed. ROM CDC have Serial 0, which is not very convenient. Changing the name I think also caused new port on my Mac.

At least on my Mac, the number after /dev/tty.usbmodem is dependent on the USB tree location of the device. It doesn't change between CircuitPython and the UF2 bootloader. (Tested on a Feather nRF52840) I can get Windows and Linux checked too.

In CircuitPython, we've implemented the 1200 baudrate trick that arduino uses.

Even if this method is used, it will still not help if the device gets a new port. And this is an ArduinoIDE solution. Will not otherwise work in IDF.

Why can't the IDF be updated to use the 1200 baudrate trick for native USB ESP boards?

For ESP32-S2 Arduino have you consider not using the ROM bootloader except in recovery? Our plan is to use a UF2 bootloader in the factory partition because mass storage bootloading is much easier for users because it requires no extra tools. (It can also still present CDC and DFU flashing as well.)

Of course all these options will be available :) Persistence could only be available if just CDC or CDC and DFU are enabled. Any other interface would turn off Persistence and trigger a reset. It makes a lot of sense to reset everything if you are updating over MSC or Vendor, but not so much with UART :)
Again, Persistence is CDC-Only thing. When DFU is also added, they both match the same descriptor as in the ROM, thus both work fine (DFU actually resets and re-enumerates).

I don't think persistence will be used much because of additional bootloaders that do more advanced protocols than cdc (like MSC.) Email me, scott@adafruit.com, and we can get you either some SAMD or nRF boards to test with. That way you can see the UF2 bootloader setup that we've been using for the last few years.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

because of additional bootloaders that do more advanced protocols than cdc (like MSC.)

Guys, I totally get all your points. You are used to a very different workflow, and from what I read in the UF2 repo, it is acting much like bootloaders on other devices.
Please consider the fact that Arduino and IDF workflow on those chips is different than yours. Things like MSC would run in the active application and not in a factory partition. Bootloaders need to be reflashed for different reasons and MSC can not do that. The only time that the chip will ever be in ROM bootloader is to do a CDC/UART flash, or DFU (which I presume will never be used in reality) and that can replace everything.

And really, please, give a good reason how having this code will harm the project? I can work around my hacks to make them more presentable, but that code will really never be triggered outside of IDF's tinyusb component and we could add define switch so that it is never otherwise compiled. I am really hoping to use upstream code and did my best to make the changes non-obtrusive. Wouldn't you prefer if you did not have to extract and hack the bootloader in order for UF2 to work on S2 :) Now you would need to keep it updated with IDF, or one day the device might not boot. This is my case here :)

we can get you either some SAMD or nRF boards to test with

thanks! I love MCUs, but hardly have time for myself nowadays. That and sending things to where I live is a quite problematic :) I see I can test the bootloader you already have for ESP32S2. Wouldn't that be the same? Also I have some local suppliers of some adafruit products, so I should be able to get something.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 1, 2020

@me-no-dev thanks for the detail breakout step, now I have a better understanding of what the persistent does and what it is trying to solve. Am I right that the sole purpose of the persistent mode is to maintain the same number of serial port (COMxx on windows, /dev/ttyACMn on Linux), so that esptool can work as the way it is ?

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

Am I right that the sole purpose of the persistent mode is to maintain the same number of serial port (COMxx on windows, /dev/ttyACMn on Linux), so that esptool can work as the way it is ?

More or less yes. That was the reason we worked on that feature :)

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 1, 2020

More or less yes. That was the reason we worked on that feature :)

Thanks for confirmation, Let's me sum it up

The Problem
Esptool couldn't upload the way it is currently using FTDI/CP210x since native USB re-enumerate

Proposed solution
You add persistent mode to trick OS thinking bootloader is application

My thought

You guys are approaching the wrong way. Here is the reasons

  1. The firmware is behaving correctly, 99 (if not 100) out of 100 devices out there will just reset and re-enumerate into bootloader. USB DFU specs even has an attribute bit specifically for this. It basically says whether device is capable of self detatch or needing host to issue a bus reset. There is really nothing wrong with firmware to fix
    image
  2. Other Arduino boards are doing native USB upload for numerous years up to now without any problems or whatsoever. It uses a well-known hack called touch1200 to put device into bootloader mode and reconnect with the new port with matched VID/PI of bootloader. You could use touch1200 supported out of the box by Arduino IDE or send DFU_DETACH request. I would suggest to just implement both. Here is how it is done just check if client disconnect with baurate = 1200 to go into bootloader https://github.com/adafruit/Adafruit_TinyUSB_ArduinoCore/blob/f96bfb250c06c447600e1befbc6601b49672d381/Adafruit_USBD_CDC.cpp#L150
  3. The persistent mode is basically treating and tricking host PC thinking we are in application mode, however we are in fact in bootloader. That will cause a nightmare support in the long term. Windows/Linux/Mac has complicated driver caching, this can easily cause hardfault memory aka dead bluescreen on windows. Although your current tests shows it works, you will likely to run into this issue occasionally.

The Right solution

The correct approach is to fix the actual problem of esptool have issue with newly enumerated port. It can be done on the PC software side, it is in face easier to deploy than firmware hack as well

  • Updating esptool to detect new port that matches your bootloader VID/PID. If you are not sure how it is done, you can take a peek at Arduino IDE implementation, they are doing it for ages.
  • Or use the touch1200 which is supported by IDE, it is mentioned here https://arduino.github.io/arduino-cli/platform-specification/#1200-bps-bootloader-reset . IDE will put your device into bootloader mode, auto detect the new port with matched VID/PID then invoke the upload tool which is esptool in your case. You can take a look at Adafruit samd/nrf boards.txt/platform.txt for reference

This is my final conclusion, don't be bothered submitting another PR for this, since It won't get approved (unless there is anything else I didn't know of).

And really, please, give a good reason how having this code will harm the project?

This is a bad reason by itself. I have no problems adding hundreds of compiler switch as long as there is a good reason behind it. Otherwise the stack won't support dozens of MCUs from multiple ARCH. The real reason for this PR is not approved is the stack DOES NOT need it and your upload workflow does not need it as well.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

thanks!

@igrr
Copy link
Copy Markdown

igrr commented Jul 1, 2020

@hathach To add to your summary, the other part of the problem that persistence solves is related to console output. When the device re-enumerates after exiting the bootloader, early log output from the application is lost. Persistence allows the host tool to keep the port open on the OS side, so as soon as the device exits the bootloader and starts sending log output, this output is fully received by the host.

Additional note on the proposed esptool change ("detect new port that matches your bootloader VID/PID"). One of the supported workflows is running multiple instances of esptool.py in parallel on a single system, for example to concurrently flash multiple development boards. Since ESP32-S2 in the bootloader mode doesn't have a serial number, we can't match the original port numbers to the re-enumerated device instances the way you propose. Keeping the serial port "alive" from OS perspective allows for this "parallel flashing" workflow.

I suppose it is not a significant issue if we don't support these aspects of the workflow when the application uses TinyUSB stack, though. If there is need to do so in the future, we will indeed try to make these changes in a fork, as you have suggested.

@tannewt
Copy link
Copy Markdown
Collaborator

tannewt commented Jul 1, 2020

Would it be enough for early output to simply use a UART and a converter chip? Seems like native USB will be enough most of the time.

For multiple esps on the same system, with Linux, at least on raspberry pi, you can select the serial port based on the usb path:

pi@broadcastnetbridge:~ $ ls /dev/serial/by-path
platform-3f980000.usb-usb-0:1.2:1.0

These are actually symlinks back to /dev/tty.

I don't know if windows has something similar.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

me-no-dev commented Jul 2, 2020

We discussed something like this with Ivan and will see what python offers on the different platforms. In a mean time I have submitted #454 to better manage the FIFO situation on S2. Will submit another small PR with a send timeout interrupt handling. A bit rare, but it happened a few times. And not that you will accept it, but persistence support has been condensed to the following changes only:
Screenshot 2020-07-02 at 0 06 19

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 7, 2020

@hathach To add to your summary, the other part of the problem that persistence solves is related to console output. When the device re-enumerates after exiting the bootloader, early log output from the application is lost. Persistence allows the host tool to keep the port open on the OS side, so as soon as the device exits the bootloader and starts sending log output, this output is fully received by the host.

Additional note on the proposed esptool change ("detect new port that matches your bootloader VID/PID"). One of the supported workflows is running multiple instances of esptool.py in parallel on a single system, for example to concurrently flash multiple development boards. Since ESP32-S2 in the bootloader mode doesn't have a serial number, we can't match the original port numbers to the re-enumerated device instances the way you propose. Keeping the serial port "alive" from OS perspective allows for this "parallel flashing" workflow.

I suppose it is not a significant issue if we don't support these aspects of the workflow when the application uses TinyUSB stack, though. If there is need to do so in the future, we will indeed try to make these changes in a fork, as you have suggested.

Hmm, this scenario of parallel flashing esp32s2 is indeed tricky. OK, if you really want to do the work, please submit an PR specifically with changes for persistence mode , I will give it a try. Maybe there is useful scenario where application-to-application reboot with persistence mode can be used later on. To me the application-bootloader is not really a good approach at all.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

The full persistence change is in the image above. No more "hacks". This is all.

There is more code, external to TinyUSB, that is actually rebooting into the proper download mode. That code is still under development/testing. Will soon look into creating a PR with the above changes and some sort of cdc example to demonstrate the feature. If you just want to see it working, it's available in the esp32s2 branch of arduino-esp32 (together with example).

I have created 2 other PRs with fixes that are more or less related to persistence. Please have a look.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 8, 2020

@me-no-dev it is hard to read, but faking the control request is much better approach. It could get approved, please prepare the PR, we could discuss more on that. We will also need a bit of comment/description describing the persistence mode and how that works in details.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

hi @hathach ! Where should that comment/description go?

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 9, 2020

Anywhere is the dcd file, don't worry about the location, just put it where you think it is appropriate. The contents is more important, we can move it around later on when needed.

@me-no-dev
Copy link
Copy Markdown
Collaborator Author

Ahh I thought you meant a separate readme.md in a doc folder or something. Got it!

@hathach
Copy link
Copy Markdown
Owner

hathach commented Jul 9, 2020

Ahh I thought you meant a separate readme.md in a doc folder or something. Got it!

That is too much to ask for, I like to throw text into my face when writing code, it makes thing less forgettable. You don't have to explain everything, just make it simple is good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants