feat: Add support for inverting reset and boot pins (ESF-177) #120

shiona · 2024-11-07T12:22:18Z

Add two new config variables SERIAL_FLASHER_RESET_INVERT and SERIAL_FLASHER_BOOT_INVERT that invert the output of their respective pins when resetting the target device.

Description

Some hardware have inverting connection between the host running the esp-serial-flasher code, and the target device boot (io0) and/or reset pin. Before this change such hardware could not be supported.

Testing

Minimal testing was done.
One STM32 device flashing an ESP32. Build system is custom and depends completely on Makefiles, so the Kconfig part was not tested at all. No other hardware has been tested.

Checklist

Before submitting a Pull Request, please ensure the following:

🚨 This PR does not introduce breaking changes.
All CI checks (GH Actions) pass.
Documentation is updated as needed.
Tests are updated or added as necessary.
Code is well-commented, especially in complex areas.
Git history is clean — commits are squashed to the minimum necessary.

shiona · 2024-11-07T12:27:08Z

I do not understand how the esp32_spi_port loader_port_enter_bootloader works. There are quite a few strap pins. Should I have added inverting configuration for all of them? Similarly I did not change anything in the esp32_usb_cdc_acm_port, since I don't know how that system works.

Dzarda7 · 2024-11-07T13:57:53Z

Thanks @shiona, nice work! Please remove the reset inverting from SPI port and mention in the documentation and Kconfig that the feature is UART only. I do not see reason to implement it to 'esp32_usb_cdc_acm_port' and if we'll agree to add this feature to 'esp32_spi_port' I will do that. Thanks.

shiona · 2024-11-07T18:40:16Z

I installed python kconfiglib and used its menuconfig to check that:

the depends on works as expected: If the selected interface is UART, then the two options are visible, if SPI or USB is selected, the options are hidden and not set.
Selecting reset invert creates a config file as expected.

Dzarda7 · 2024-11-07T19:41:21Z

README.md

+* `SERIAL_FLASHER_RESET_INVERT`
+
+This inverts the output of the reset gpio pin. Useful if the hardware has inverting connection
+between the host and the target reset pin. Works only on UART interface.


Just a nitpick.

Suggested change

between the host and the target reset pin. Works only on UART interface.

between the host and the target reset pin. Implemented only for UART interface.

Dzarda7 · 2024-11-07T19:41:45Z

README.md

+
+* `SERIAL_FLASHER_BOOT_INVERT`
+This inverts the output of the boot (IO0) gpio pin. Useful if the hardware has inverting connection
+between the host and the target boot pin. Works only on UART interface.


Suggested change

between the host and the target boot pin. Works only on UART interface.

between the host and the target boot pin. Implemented only for UART interface..

Dzarda7 · 2024-11-07T19:46:35Z

Please squash the second commit to the first one. I proposed just a little nitpick. Did a quick test and I like it, thanks for finding out the depends on option.

Thank you for contribution. I will test it properly and merge it as soon as possible.

Add two new config variables SERIAL_FLASHER_RESET_INVERT and SERIAL_FLASHER_BOOT_INVERT that invert the output of their respective pins when resetting the target device. Only implemented for UART interface.

shiona · 2024-11-07T20:33:27Z

I would have squashed them earlier, but the PR template specifically requests not force pushing after review has started.
If you prefer possible future contributors to squash the edits on their own initiative, you might want to edit the wording in the template.

Dzarda7 · 2024-11-08T07:38:22Z

Okay, thanks for letting me know.

Dzarda7 · 2024-11-08T14:24:57Z

I tried to build it and found few problems unfortunately. Main problem is that bool type option is not present in sdkconfig.h when set to n. I will write the changes using suggestions, but feel free to let me know that you do not want to do that and I will implement it by myself.

Dzarda7 · 2024-11-08T14:28:25Z

Kconfig

+    config SERIAL_FLASHER_RESET_INVERT
+        bool "Invert reset signal"
+        default n
+        depends on SERIAL_FLASHER_INTERFACE_UART
+        help
+           Enable this option if there is an inverting connection between
+           the output of the serial-flasher and the reset pin of the ESP chip.
+
+    config SERIAL_FLASHER_BOOT_INVERT
+        bool "Invert boot signal"
+        default n
+        depends on SERIAL_FLASHER_INTERFACE_UART
+        help
+           Enable this option if there is an inverting connection between
+           the output of the serial-flasher and the boot pin of the ESP chip.


Suggested change

config SERIAL_FLASHER_RESET_INVERT

bool "Invert reset signal"

default n

depends on SERIAL_FLASHER_INTERFACE_UART

help

Enable this option if there is an inverting connection between

the output of the serial-flasher and the reset pin of the ESP chip.

config SERIAL_FLASHER_BOOT_INVERT

bool "Invert boot signal"

default n

depends on SERIAL_FLASHER_INTERFACE_UART

help

Enable this option if there is an inverting connection between

the output of the serial-flasher and the boot pin of the ESP chip.

config SERIAL_FLASHER_RESET_ACTIVE_LEVEL

int "Set the active level of the reset pin"

range 0 1

default 0

depends on SERIAL_FLASHER_INTERFACE_UART

help

Set the active level of the reset pin. 0 for low active, 1 for high active.

config SERIAL_FLASHER_BOOT_ACTIVE_LEVEL

int "Set the active level of the boot pin"

range 0 1

default 0

depends on SERIAL_FLASHER_INTERFACE_UART

help

Set the active level of the boot pin. 0 for low active, 1 for high active.

I actually don't see a problem with the current naming, but this is fine as well.

Dzarda7 · 2024-11-08T14:29:06Z

README.md

+* `SERIAL_FLASHER_RESET_INVERT`
+
+This inverts the output of the reset gpio pin. Useful if the hardware has inverting connection
+between the host and the target reset pin. Implemented only for UART interface.
+
+Default: n
+
+* `SERIAL_FLASHER_BOOT_INVERT`
+This inverts the output of the boot (IO0) gpio pin. Useful if the hardware has inverting connection
+between the host and the target boot pin. Implemented only for UART interface.
+
+Default: n
+


Rewrite appropriately.

Dzarda7 · 2024-11-08T14:31:00Z

port/esp32_port.c

@@ -142,18 +142,18 @@ esp_loader_error_t loader_port_read(uint8_t *data, uint16_t size, uint32_t timeo
 // assert reset pin for 50 milliseconds.
 void loader_port_enter_bootloader(void)
 {
-    gpio_set_level(s_gpio0_trigger_pin, 0);
+    gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 1 : 0);


Suggested change

gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 1 : 0);

gpio_set_level(s_gpio0_trigger_pin, CONFIG_SERIAL_FLASHER_BOOT_ACTIVE_LEVEL);

esp32_port.c refers to SERIAL_FLASHER_RESET_HOLD_TIME_MS as such without CONFIG_ prefix.
Is there something I don't understand why this new option would behave differently?

This same question goes for the other comments too.

Hi @shiona, this is due to the fact that KConfig options do not get simply passed as defines to the build in ESP-IDF. KConfig options are processed and then CMake variables are created from them, which have to be handled in the build system.

These CMake variables will have the CONFIG prefix, unlike CMake variables for non-esp ports.

You can see that we have created a convenience macro for adding options which handles those differences and also the n and y booleans from KConfig, please use this to add the option.

Dzarda7 · 2024-11-08T14:31:28Z

port/esp32_port.c

    loader_port_reset_target();
    loader_port_delay_ms(SERIAL_FLASHER_BOOT_HOLD_TIME_MS);
-    gpio_set_level(s_gpio0_trigger_pin, 1);
+    gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 0 : 1);


Suggested change

gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 0 : 1);

gpio_set_level(s_gpio0_trigger_pin, !CONFIG_SERIAL_FLASHER_BOOT_ACTIVE_LEVEL);

Dzarda7 · 2024-11-08T14:31:44Z

port/esp32_port.c

 }


 void loader_port_reset_target(void)
 {
-    gpio_set_level(s_reset_trigger_pin, 0);
+    gpio_set_level(s_reset_trigger_pin, SERIAL_FLASHER_RESET_INVERT ? 1 : 0);


Suggested change

gpio_set_level(s_reset_trigger_pin, SERIAL_FLASHER_RESET_INVERT ? 1 : 0);

gpio_set_level(s_reset_trigger_pin, CONFIG_SERIAL_FLASHER_RESET_ACTIVE_LEVEL);

I actually believe the initial approach is more readable and future-proof, as well as being consistent with the rest of the ports.

Dzarda7 · 2024-11-08T14:31:55Z

port/esp32_port.c

    loader_port_delay_ms(SERIAL_FLASHER_RESET_HOLD_TIME_MS);
-    gpio_set_level(s_reset_trigger_pin, 1);
+    gpio_set_level(s_reset_trigger_pin, SERIAL_FLASHER_RESET_INVERT ? 0 : 1);


gpio_set_level(s_reset_trigger_pin, !CONFIG_SERIAL_FLASHER_RESET_ACTIVE_LEVEL);

Dzarda7 · 2024-11-08T14:37:15Z

You can use logic inversion in ports where the pin status is set using 0 and 1. In ports which have own definition (like STM32) use conditional operator.

Dzarda7 · 2024-11-13T09:18:22Z

Hi @shiona, sorry for confusion. I did not know about the macro so thanks @DNedic for help. As @DNedic proposed, it should be enough to keep changes you already did and use the macro in CMakeLists.txt on the new options, I already tried it and it seems to work. Just please remove the comment above void loader_port_enter_bootloader(void) function in esp32_port.c which will not be true with this new feature. Thanks and sorry once again.

github-actions bot changed the title ~~feat: Add support for inverting reset and boot pins~~ feat: Add support for inverting reset and boot pins (ESF-177) Nov 7, 2024

Dzarda7 reviewed Nov 7, 2024

View reviewed changes

feat: Add support for inverting reset and boot pins

848a0ab

Add two new config variables SERIAL_FLASHER_RESET_INVERT and SERIAL_FLASHER_BOOT_INVERT that invert the output of their respective pins when resetting the target device. Only implemented for UART interface.

shiona force-pushed the reset-boot-invert-option branch from 8de5511 to 848a0ab Compare November 7, 2024 20:30

Dzarda7 reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for inverting reset and boot pins (ESF-177) #120

feat: Add support for inverting reset and boot pins (ESF-177) #120

shiona commented Nov 7, 2024 •

edited

Loading

shiona commented Nov 7, 2024

Dzarda7 commented Nov 7, 2024

shiona commented Nov 7, 2024

Dzarda7 Nov 7, 2024

Dzarda7 Nov 7, 2024

Dzarda7 commented Nov 7, 2024

shiona commented Nov 7, 2024

Dzarda7 commented Nov 8, 2024

Dzarda7 commented Nov 8, 2024 •

edited

Loading

Dzarda7 Nov 8, 2024

DNedic Nov 12, 2024

Dzarda7 Nov 8, 2024

Dzarda7 Nov 8, 2024

shiona Nov 8, 2024

DNedic Nov 12, 2024

Dzarda7 Nov 8, 2024

Dzarda7 Nov 8, 2024

DNedic Nov 12, 2024 •

edited

Loading

Dzarda7 Nov 8, 2024

Dzarda7 commented Nov 8, 2024

Dzarda7 commented Nov 13, 2024

	between the host and the target reset pin. Works only on UART interface.
	between the host and the target reset pin. Implemented only for UART interface.

	between the host and the target boot pin. Works only on UART interface.
	between the host and the target boot pin. Implemented only for UART interface..

	gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 1 : 0);
	gpio_set_level(s_gpio0_trigger_pin, CONFIG_SERIAL_FLASHER_BOOT_ACTIVE_LEVEL);

	gpio_set_level(s_gpio0_trigger_pin, SERIAL_FLASHER_BOOT_INVERT ? 0 : 1);
	gpio_set_level(s_gpio0_trigger_pin, !CONFIG_SERIAL_FLASHER_BOOT_ACTIVE_LEVEL);

	gpio_set_level(s_reset_trigger_pin, SERIAL_FLASHER_RESET_INVERT ? 1 : 0);
	gpio_set_level(s_reset_trigger_pin, CONFIG_SERIAL_FLASHER_RESET_ACTIVE_LEVEL);

feat: Add support for inverting reset and boot pins (ESF-177) #120

Are you sure you want to change the base?

feat: Add support for inverting reset and boot pins (ESF-177) #120

Conversation

shiona commented Nov 7, 2024 • edited Loading

Description

Related

Testing

Checklist

shiona commented Nov 7, 2024

Dzarda7 commented Nov 7, 2024

shiona commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dzarda7 commented Nov 7, 2024

shiona commented Nov 7, 2024

Dzarda7 commented Nov 8, 2024

Dzarda7 commented Nov 8, 2024 • edited Loading

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

DNedic Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dzarda7 commented Nov 8, 2024

Dzarda7 commented Nov 13, 2024

shiona commented Nov 7, 2024 •

edited

Loading

Dzarda7 commented Nov 8, 2024 •

edited

Loading

DNedic Nov 12, 2024 •

edited

Loading