Skip to content

ports/esp32: Add basic espressif IDF v5.3 compatibility. #15733

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 2 commits into from
Dec 10, 2024

Conversation

andrewleech
Copy link
Contributor

Summary

Espressif have recently release IDF v5.3, this PR is working towards basic compatibility with it.

Testing

I've initially tried running this on a C3 device which only has the builtin USB exposed (no uart->usb).
I do not get repl on the USB connection however, nothing is seen over the USB port after the initial reboot / startup text:

--- esp-idf-monitor 1.4.0 on /dev/ttyUSB0 460800 ---
--- Quit: Ctrl+] | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H ---
ESP-ROM:esp32c3-api1-20210207
Build:Feb  7 2021
rst:0x15 (USB_UART_CHIP_RESET),boot:0xf (SPI_FAST_FLASH_BOOT)
Saved PC:0x4200832c
0x4200832c: usb_serial_jtag_ll_txfifo_writable at /opt/esp/idf/components/hal/esp32c3/include/hal/usb_serial_jtag_ll.h:160
 (inlined by) usb_serial_jtag_tx_strn at /home/anl/micropython/ports/esp32/usb_serial_jtag.c:109

SPIWP:0xee
mode:DIO, clock div:1
load:0x3fcd5820,len:0xf28
load:0x403cc710,len:0x93c
load:0x403ce710,len:0x2ac8
entry 0x403cc710

Trade-offs and Alternatives

Unknown at this point.

Copy link

github-actions bot commented Aug 28, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (a8d1c25) to head (a20171b).
Report is 14 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #15733   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files         163      163           
  Lines       21295    21295           
=======================================
  Hits        20961    20961           
  Misses        334      334           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -95,6 +95,10 @@ void usb_serial_jtag_poll_rx(void) {
}
}

#ifndef USB_SERIAL_JTAG_PACKET_SZ_BYTES
#define USB_SERIAL_JTAG_PACKET_SZ_BYTES USB_SERIAL_JTAG_RDWR_BYTE_S
Copy link
Contributor

@projectgus projectgus Aug 28, 2024

Choose a reason for hiding this comment

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

The value of USB_SERIAL_JTAG_RDWR_BYTE_S is 0 which is why you don't get any bytes on stdout any more.

Try defining to 64 for now, that was the value before the macro was removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that!
I did scan the 5.3 migration guide and didn't see anything about that but I guess USB_SERIAL_JTAG_RDWR_BYTE_S wasn't necessarily ever intended as an end user/application variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, talking directly to the usb serial peripheral (rather than via stdio) is a private "low level" interface so no guarantees, I think!

@projectgus
Copy link
Contributor

I updated to the release/v5.2 branch tip (which is how I came across the usb jtag constant thing) and also needed this patch for sockets to work. It seems now ai_canonname can come back as NULL for getaddrinfo. You'll probably need something like this for v5.3:

diff --git i/ports/esp32/modsocket.c w/ports/esp32/modsocket.c
index 916eb79bd9..641bcd1170 100644
--- i/ports/esp32/modsocket.c
+++ w/ports/esp32/modsocket.c
@@ -253,7 +253,7 @@ static void _getaddrinfo_inner(const mp_obj_t host, const mp_obj_t portx,
     // Somehow LwIP returns a resolution of 0.0.0.0 for failed lookups, traced it as far back
     // as netconn_gethostbyname_addrtype returning OK instead of error.
     if (*res == NULL ||
-        (strcmp(res[0]->ai_canonname, "0.0.0.0") == 0 && strcmp(host_str, "0.0.0.0") != 0)) {
+        (res[0]->ai_canonname && strcmp(res[0]->ai_canonname, "0.0.0.0") == 0 && strcmp(host_str, "0.0.0.0") != 0)) {
         lwip_freeaddrinfo(*res);
         mp_raise_OSError(-2); // name or service not known
     }
@@ -261,7 +261,7 @@ static void _getaddrinfo_inner(const mp_obj_t host, const mp_obj_t portx,
     assert(retval == 0 && *res != NULL);
 }
 
-static void _socket_getaddrinfo(const mp_obj_t addrtuple, struct addrinfo **resp) {
+static void _socket_getaddrinfo const mp_obj_t addrtuple, struct addrinfo **resp) {
     mp_obj_t *elem;
     mp_obj_get_array_fixed_n(addrtuple, 2, &elem);
     _getaddrinfo_inner(elem[0], elem[1], NULL, resp);
@@ -951,7 +951,7 @@ static mp_obj_t esp_socket_getaddrinfo(size_t n_args, const mp_obj_t *args) {
             mp_obj_new_int(resi->ai_family),
             mp_obj_new_int(resi->ai_socktype),
             mp_obj_new_int(resi->ai_protocol),
-            mp_obj_new_str_from_cstr(resi->ai_canonname),
+            resi->ai_canonname ? mp_obj_new_str_from_cstr(resi->ai_canonname) : mp_const_empty_bytes,
             mp_const_none
         };
 

@projectgus
Copy link
Contributor

Oh weird, I saw that you have v5.3 working maybe without the sockets change! Guess not...

Copy link
Contributor

@DvdGiessen DvdGiessen 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! Can confirm seems to at least start up and have basic working functionality with the ESP32, ESP32-C3 and ESP32-S3 boards I just now flashed with these changes.

Changed are almost identical to my own (uncommited) changes to support 5.3 from a month ago. Can confirm that indeed the 64 bytes JTAG serial size is no longer available (removed here), for my changes I did go through the IDF internals a bit to check and the size seems always be 64 for every chip variant, so hardcoding it on our side should be fine.

Two differences that might be worth considering: I did also change the JTAG serial RX buffer to use the packet size instead of its own constant, since I didn't see a reason it should use a different value. And, after looking through NimBLE and seeing ediv_random was fully removed there, I figured we can probably remove all references to it altogether. And, should we also be updating the list of constants in network_wlan.c?

(My own uncommited changes)
diff --git a/extmod/nimble/modbluetooth_nimble.c b/extmod/nimble/modbluetooth_nimble.c
index f582661ff..9cb12ee77 100644
--- a/extmod/nimble/modbluetooth_nimble.c
+++ b/extmod/nimble/modbluetooth_nimble.c
@@ -1911,24 +1911,21 @@ static int ble_secret_store_read(int obj_type, const union ble_store_key *key, u
                 // <type=peer,addr,*> (single)
                 // Find the entry for this specific peer.
                 assert(key->sec.idx == 0);
-                // assert(!key->sec.ediv_rand_present);
                 key_data = (const uint8_t *)&key->sec.peer_addr;
                 key_data_len = sizeof(ble_addr_t);
             } else {
                 // <type=peer,*> (with index)
                 // Iterate all known peers.
-                // assert(!key->sec.ediv_rand_present);
                 key_data = NULL;
                 key_data_len = 0;
             }
             break;
         }
         case BLE_STORE_OBJ_TYPE_OUR_SEC: {
-            // <type=our,addr,ediv_rand>
-            // Find our secret for this remote device, matching this ediv/rand key.
+            // <type=our,addr,*>
+            // Find our secret for this remote device.
             assert(ble_addr_cmp(&key->sec.peer_addr, BLE_ADDR_ANY)); // Must have address.
             assert(key->sec.idx == 0);
-            // assert(key->sec.ediv_rand_present);
             key_data = (const uint8_t *)&key->sec.peer_addr;
             key_data_len = sizeof(ble_addr_t);
             break;
@@ -1958,10 +1955,6 @@ static int ble_secret_store_read(int obj_type, const union ble_store_key *key, u
 
     DEBUG_printf("ble_secret_store_read: found secret\n");
 
-    if (obj_type == BLE_STORE_OBJ_TYPE_OUR_SEC) {
-        // TODO: Verify ediv_rand matches.
-    }
-
     return 0;
 }
 
@@ -1970,14 +1963,13 @@ static int ble_secret_store_write(int obj_type, const union ble_store_value *val
     switch (obj_type) {
         case BLE_STORE_OBJ_TYPE_PEER_SEC:
         case BLE_STORE_OBJ_TYPE_OUR_SEC: {
-            // <type=peer,addr,edivrand>
+            // <type=peer,addr,*>
 
             struct ble_store_key_sec key_sec;
             const struct ble_store_value_sec *value_sec = &val->sec;
             ble_store_key_from_value_sec(&key_sec, value_sec);
 
             assert(ble_addr_cmp(&key_sec.peer_addr, BLE_ADDR_ANY)); // Must have address.
-            // assert(key_sec.ediv_rand_present);
 
             if (!mp_bluetooth_gap_on_set_secret(obj_type, (const uint8_t *)&key_sec.peer_addr, sizeof(ble_addr_t), (const uint8_t *)value_sec, sizeof(struct ble_store_value_sec))) {
                 DEBUG_printf("Failed to write key: type=%d\n", obj_type);
@@ -2007,7 +1999,6 @@ static int ble_secret_store_delete(int obj_type, const union ble_store_key *key)
             // <type=peer,addr,*>
 
             assert(ble_addr_cmp(&key->sec.peer_addr, BLE_ADDR_ANY)); // Must have address.
-            // ediv_rand is optional (will not be present for delete).
 
             if (!mp_bluetooth_gap_on_set_secret(obj_type, (const uint8_t *)&key->sec.peer_addr, sizeof(ble_addr_t), NULL, 0)) {
                 DEBUG_printf("Failed to delete key: type=%d\n", obj_type);
diff --git a/ports/esp32/usb_serial_jtag.c b/ports/esp32/usb_serial_jtag.c
index 23a73d053..709b93e85 100644
--- a/ports/esp32/usb_serial_jtag.c
+++ b/ports/esp32/usb_serial_jtag.c
@@ -35,10 +35,12 @@
 #include "soc/periph_defs.h"
 #include "freertos/portmacro.h"
 
-#define USB_SERIAL_JTAG_BUF_SIZE (64)
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 3, 0)
+#define USB_SERIAL_JTAG_PACKET_SZ_BYTES 64
+#endif
 
 static DRAM_ATTR portMUX_TYPE rx_mux = portMUX_INITIALIZER_UNLOCKED;
-static uint8_t rx_buf[USB_SERIAL_JTAG_BUF_SIZE];
+static uint8_t rx_buf[USB_SERIAL_JTAG_PACKET_SZ_BYTES];
 static volatile bool terminal_connected = false;
 
 static void usb_serial_jtag_handle_rx(void) {
@@ -48,8 +50,8 @@ static void usb_serial_jtag_handle_rx(void) {
         portENTER_CRITICAL(&rx_mux);
     }
     size_t req_len = ringbuf_free(&stdin_ringbuf);
-    if (req_len > USB_SERIAL_JTAG_BUF_SIZE) {
-        req_len = USB_SERIAL_JTAG_BUF_SIZE;
+    if (req_len > USB_SERIAL_JTAG_PACKET_SZ_BYTES) {
+        req_len = USB_SERIAL_JTAG_PACKET_SZ_BYTES;
     }
     size_t len = usb_serial_jtag_ll_read_rxfifo(rx_buf, req_len);
     for (size_t i = 0; i < len; ++i) {
@@ -95,10 +97,6 @@ void usb_serial_jtag_poll_rx(void) {
     }
 }
 
-#ifndef USB_SERIAL_JTAG_PACKET_SZ_BYTES
-#define USB_SERIAL_JTAG_PACKET_SZ_BYTES USB_SERIAL_JTAG_RDWR_BYTE_S
-#endif
-
 void usb_serial_jtag_tx_strn(const char *str, size_t len) {
     while (len) {
         size_t l = len;

@andrewleech
Copy link
Contributor Author

Thanks for sharing your changes, I'll compare a bit later when I get a chance

should we also be updating the list of constants in network_wlan.c

I wondered that too, it like like they weren't updated for the last couple of versions either. Reflecting these as qstr's does take up flash space so I guess it depends on whether these are things end users are likely to need to reference?

@andrewleech
Copy link
Contributor Author

Oh weird, I saw that you have v5.3 working maybe without the sockets change! Guess not...

Yeah I guess sockets worked for the mqtt comms, that was only trialled on C3 though, and not for long. Perhaps there's still other situations a null would need to be checked for / handled

@DvdGiessen
Copy link
Contributor

DvdGiessen commented Sep 1, 2024

the list of constants in network_wlan.c

they weren't updated for the last couple of versions either

Since I did the v5.2 PR (#13775) I looked into why I didn't update them there, and the reason is they didn't exist yet, they were added later in #14015 (or to be more precise, these PR's happened in parallel, so I added the new 5.2 constants but didn't add them to network_wlan.c, and @iabdalkader added the constants there but didn't include the v5.2 changes).

If I'm reading it correctly, the decision made in the discussion on that PR was that these constants should live as part of the WLAN class, and thus the network module constants are actually just there for backward compatibility; but are slated to be removed in MicroPython 2.0.

If that's the case, then we should probably:

  • not add the constants to network
  • add them to the WLAN class instead
  • add a comment to in network's source code explaining that those are there only for backward compatibility and will be removed in 2.0 (do we have a specific annotation or anything for those so they're not forgotten about when the time to release 2.0 comes?)
  • move the compile-time checks from network to the WLAN class so that the build will actually break there and those are the ones we cannot forget to update going forward?

@dpgeorge
Copy link
Member

dpgeorge commented Sep 2, 2024

these constants should live as part of the WLAN class, and thus the network module constants are actually just there for backward compatibility

That is correct.

If that's the case, then we should probably:
...

Yes, I agree with this list of TODOs (see comment immediately above).

do we have a specific annotation or anything for those so they're not forgotten about when the time to release 2.0 comes?

Yes, MICROPY_PREVIEW_VERSION_2.

@DuaneKaufman
Copy link

Dear All,
I would like to sponsor moving this particular issue along, to un-block: #15731

Is it possible to sponsor a specific effort?

Sincerely,
Duane Kaufman

@andrewleech
Copy link
Contributor Author

I'm in the middle of other USB work which is taking up my limited personal time for this kind of work, as such I don't when I'd have time to do more with this PR.

It seems like there probably isn't much more work needed though? I'd be quite happy if anyone else wanted to continue working on it or even just testing what's missing here?

@DuaneKaufman
Copy link

DuaneKaufman commented Oct 30, 2024

I'm in the middle of other USB work which is taking up my limited personal time for this kind of work, as such I don't when I'd have time to do more with this PR.

It seems like there probably isn't much more work needed though? I'd be quite happy if anyone else wanted to continue working on it or even just testing what's missing here?

Hi All,

Unfortunately, I am not familiar with the PR process at all....though I would be happy to test if someone told me how.

Sincerely,
Duane

@dpgeorge
Copy link
Member

  • not add the constants to network

  • add them to the WLAN class instead

  • add a comment to in network's source code explaining that those are there only for backward compatibility and will be removed in 2.0 (do we have a specific annotation or anything for those so they're not forgotten about when the time to release 2.0 comes?)

  • move the compile-time checks from network to the WLAN class so that the build will actually break there and those are the ones we cannot forget to update going forward?

I've now done this, see #16119.

@dpgeorge
Copy link
Member

I removed the ediv_rand_present asserts in #16120.

@dpgeorge
Copy link
Member

I've rebased this on latest master, cleaned up a few things, and force pushed.

Tested on ESP32_GENERIC. Everything seems to work except thread/mutate_bytearray.py when run as native code, it locks up.

Tested on ESP32_GENERIC_S2, that also locks up trying to run some native code (although I think that's a problem on master...).

@dpgeorge
Copy link
Member

dpgeorge commented Nov 5, 2024

I rebased this on master to get the above-mentioned patches. This PR is now quite a bit simpler.

@DvdGiessen
Copy link
Contributor

Just updated my tree with these commits, building against the latest IDF v5.3.1, and can confirm it seems to be running well so far.

@dpgeorge dpgeorge added this to the release-1.25.0 milestone Nov 5, 2024
@andrewleech andrewleech changed the title DRAFT: ports/esp32: Add basic espressif IDF v5.3 compatibility. ports/esp32: Add basic espressif IDF v5.3 compatibility. Nov 6, 2024
@DvdGiessen
Copy link
Contributor

Note: #16169 has broken building against 5.3 due to espressif/esp-idf@f35ec64 having renamed the SOC_TOUCH_VERSION_x macros.

@DvdGiessen
Copy link
Contributor

DvdGiessen commented Dec 3, 2024

A small change that seems to fix that:

0001-esp32-Fix-machine_touchpad-compiling-on-IDFv5.3.patch
From 49de284d2896bf209fe255a26c304af55ba13fa2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dani=C3=ABl=20van=20de=20Giessen?= <daniel@dvdgiessen.nl>
Date: Tue, 3 Dec 2024 11:08:13 +0100
Subject: [PATCH] esp32: Fix machine_touchpad compiling on IDFv5.3.

---
 ports/esp32/machine_touchpad.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/ports/esp32/machine_touchpad.c b/ports/esp32/machine_touchpad.c
index 48250280b..299c489f5 100644
--- a/ports/esp32/machine_touchpad.c
+++ b/ports/esp32/machine_touchpad.c
@@ -31,9 +31,17 @@
 
 #if SOC_TOUCH_SENSOR_SUPPORTED
 
-#if SOC_TOUCH_VERSION_1 // ESP32 only
+#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 3, 0)
+#if SOC_TOUCH_VERSION_1
+#define SOC_TOUCH_SENSOR_VERSION (1)
+#elif SOC_TOUCH_VERSION_2
+#define SOC_TOUCH_SENSOR_VERSION (2)
+#endif
+#endif
+
+#if SOC_TOUCH_SENSOR_VERSION == 1 // ESP32 only
 #include "driver/touch_pad.h"
-#elif SOC_TOUCH_VERSION_2 // All other SoCs with touch, to date
+#elif SOC_TOUCH_SENSOR_VERSION == 2 // All other SoCs with touch, to date
 #include "driver/touch_sensor.h"
 #else
 #error "Unknown touch hardware version"
@@ -98,13 +106,13 @@ static mp_obj_t mtp_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_
         touch_pad_set_fsm_mode(TOUCH_FSM_MODE_TIMER);
         initialized = 1;
     }
-    #if SOC_TOUCH_VERSION_1
+    #if SOC_TOUCH_SENSOR_VERSION == 1
     esp_err_t err = touch_pad_config(self->touchpad_id, 0);
-    #elif SOC_TOUCH_VERSION_2
+    #elif SOC_TOUCH_SENSOR_VERSION == 2
     esp_err_t err = touch_pad_config(self->touchpad_id);
     #endif
     if (err == ESP_OK) {
-        #if SOC_TOUCH_VERSION_2
+        #if SOC_TOUCH_SENSOR_VERSION == 2
         touch_pad_fsm_start();
         #endif
 
@@ -115,10 +123,10 @@ static mp_obj_t mtp_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_
 
 static mp_obj_t mtp_config(mp_obj_t self_in, mp_obj_t value_in) {
     mtp_obj_t *self = self_in;
-    #if SOC_TOUCH_VERSION_1
+    #if SOC_TOUCH_SENSOR_VERSION == 1
     uint16_t value = mp_obj_get_int(value_in);
     esp_err_t err = touch_pad_config(self->touchpad_id, value);
-    #elif SOC_TOUCH_VERSION_2
+    #elif SOC_TOUCH_SENSOR_VERSION == 2
     esp_err_t err = touch_pad_config(self->touchpad_id);
     #endif
     if (err == ESP_OK) {
@@ -130,10 +138,10 @@ MP_DEFINE_CONST_FUN_OBJ_2(mtp_config_obj, mtp_config);
 
 static mp_obj_t mtp_read(mp_obj_t self_in) {
     mtp_obj_t *self = self_in;
-    #if SOC_TOUCH_VERSION_1
+    #if SOC_TOUCH_SENSOR_VERSION == 1
     uint16_t value;
     esp_err_t err = touch_pad_read(self->touchpad_id, &value);
-    #elif SOC_TOUCH_VERSION_2
+    #elif SOC_TOUCH_SENSOR_VERSION == 2
     uint32_t value;
     esp_err_t err = touch_pad_read_raw_data(self->touchpad_id, &value);
     #endif
-- 
2.47.1

@dpgeorge
Copy link
Member

dpgeorge commented Dec 4, 2024

Thanks @DvdGiessen , I have applied your patch. I also rebased this on latest master.

@projectgus I think we can merge this, what do you think? We won't official move to 5.3 yet so don't need to exhaustively test this with that IDF version. And the changes should not affect IDF<5.3.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Agree. The only thing I (just) noticed missing is adding v5.3 to the list of supported versions in the README.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

The only thing I (just) noticed missing is adding v5.3 to the list of supported versions in the README.

@dpgeorge I went ahead and rebased again to add this, so I think it's good to go!

@projectgus
Copy link
Contributor

Note as we're adding a version that there is also an open PR to drop older versions - #16128

(Will rebase after this is merged.)

@DvdGiessen
Copy link
Contributor

FWIW I just built with these changes against the newly released IDF v5.3.2 and that also runs fine. :)

@keenanjohnson
Copy link
Contributor

I also did a quick test with a project using an esp32-s3 and v5.3.2 with this branch and everything seemed to work as expected.

pi-anl and others added 2 commits December 10, 2024 11:20
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge merged commit 5fb846d into micropython:master Dec 10, 2024
8 checks passed
@dpgeorge
Copy link
Member

Merged. Thanks everyone who worked on this.

@keenanjohnson
Copy link
Contributor

Yes thank you all! I'm an avid user of the esp32 + micropython and unglamorous tasks like this really help me out. I appreciate you!

@DuaneKaufman
Copy link

DuaneKaufman commented Dec 10, 2024 via email

@andrewleech andrewleech deleted the idf-5.3 branch December 18, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants