Update esp-idf to v4.3 #4195

askpatrickw · 2021-02-13T19:06:52Z

Starting the work for #4182

Updated the branch ESP-IDF to point to releases/v4.3
Fixed the include paths in Makefile
Fixed #includes which had incorrect full paths to use shorter paths

Open Questions:

There are other full path includes.. they work, but should they be changed?
Local build now fails on actual code changes which would be needed.

This is the first one, but there are more errors behind this. It would be great if someone who knows C could pickup from here.

common-hal/busio/I2C.c: In function 'common_hal_busio_i2c_construct':
common-hal/busio/I2C.c:93:56: error: comparison of distinct pointer types lacks a cast [-Werror]
     if (xSemaphoreCreateBinaryStatic(&self->semaphore) != &self->semaphore) {
                                                        ^~
In file included from ./common-hal/busio/I2C.h:34,
                 from ../../shared-bindings/busio/I2C.h:40,
                 from common-hal/busio/I2C.c:27:
common-hal/busio/I2C.c: In function 'common_hal_busio_i2c_try_lock':
common-hal/busio/I2C.c:169:37: error: passing argument 1 of 'xQueueSemaphoreTake' from incompatible pointer type [-Werror=incompatible-pointer-types]
     self->has_lock = xSemaphoreTake(&self->semaphore, 0) == pdTRUE;
                                     ^
In file included from esp-idf/components/freertos/include/freertos/semphr.h:35,
                 from ./common-hal/busio/I2C.h:34,
                 from ../../shared-bindings/busio/I2C.h:40,
                 from common-hal/busio/I2C.c:27:
esp-idf/components/freertos/include/freertos/queue.h:1239:47: note: expected 'QueueHandle_t' {aka 'struct QueueDefinition *'} but argument is of type 'StaticSemaphore_t *' {aka 'struct xSTATIC_QUEUE *'}
 BaseType_t xQueueSemaphoreTake( QueueHandle_t xQueue, TickType_t xTicksToWait ) PRIVILEGED_FUNCTION;
                                 ~~~~~~~~~~~~~~^~~~~~
cc1: all warnings being treated as errors
make: *** [build-unexpectedmaker_feathers2/common-hal/busio/I2C.o] Error 1

skieast · 2021-02-13T22:34:33Z

I also started this chase down the rabbit hole. I however did not make nice edits so my result is not really pretty.
The semaphore stuff is easy to fix. Once that is done the SPI requires some relatively major changes as Espressif have refactored some calls. I made some changes so it compiles but doesnt work. I will now start again :)

I'm not really liking all the full path includes. I changed them to most;ly match how the IDF does its includes. I think this might make it easier for future IDF releases.

I had started this to see if the newer IDF release fixed anything with I2C, havent got that far yet.

askpatrickw · 2021-02-13T22:44:33Z

Thanks @skieast, I appreciate you picking this up and running with it !

skieast · 2021-02-14T01:02:26Z

Thanks @skieast, I appreciate you picking this up and running with it !

tbh I have no idea how to 'run with it'. Does that somehow involve me modifying your PR? Or do I just start a new PR with what I have. As you can probably see my github knowledge is limited. Very much so.
I may just start a new local branch and make my changes in a more consistent way now that I know it works.

askpatrickw · 2021-02-14T01:12:19Z

@skieast I think you can use the gh cli to checkout the PR.
gh pr checkout 4195 But maybe you need to be a maintainer on my fork ... hmmm not sure now that I say that outloud.

skieast · 2021-02-14T01:15:24Z

I'll do some reading. Forgot about the gh cli. In any case I'm going to at least try to get spi working here. Thanks.

microdev1 · 2021-02-14T03:35:04Z

I should have made a PR earlier lol...
I have a compiling version available in my update-idf branch.
Didn't make PR because SPI and I2C stuff needed some testing. How should we proceed now?

skieast · 2021-02-14T03:46:48Z

I should have made a PR earlier lol...
I have a compiling version available in my update-idf branch.
Didn't make PR because SPI and I2C stuff needed some testing. How should we proceed now?

Awesome. I will take a look at your branch. If I have time later tomorrow I'll do some i2c testing.

askpatrickw · 2021-02-14T19:11:32Z

On the Discord it was suggested you could fork my fork, but I know you can't do that really without removing other forks. I could make you collaborators on my fork instead.

But, why don't we abandon this PR and you can do your own sounds like @microdev1 had already gotten farther than I had anyway. That's probably the easiest. I'll leave this open and you all can decide. Thanks for doing the work for this!

hierophect · 2021-02-16T18:51:39Z

How's the progress on this coming along? I'd like to update in order to try and make more progress on the ongoing I2C problems, it'd be great if we could bring everyone's progress together. To clone someone else's branch, you can follow the instructions here, it's not too bad, but you do need to make sure your PR is marked to accept contributions.

skieast · 2021-02-16T19:01:30Z

I’m out now. As I posted on discord the branch from microdev compiles but had some issues. I can get i2c scan to work but otherwise it is very unstable. Will try some code later to see if crashing is general.

tannewt · 2021-02-16T23:01:29Z

I'm not really liking all the full path includes. I changed them to most;ly match how the IDF does its includes. I think this might make it easier for future IDF releases.

Why not? I like the full path because that makes them easier to find. I dislike having super long include directory lists and include filenames that look like they come from the same place but don't.

On the Discord it was suggested you could fork my fork, but I know you can't do that really without removing other forks. I could make you collaborators on my fork instead.

You can always add each other as remotes to your local repo:

git remote add <remote name> <git url>

askpatrickw · 2021-02-17T22:51:52Z

I'm not really liking all the full path includes. I changed them to most;ly match how the IDF does its includes. I think this might make it easier for future IDF releases.

Why not? I like the full path because that makes them easier to find. I dislike having super long include directory lists and include filenames that look like they come from the same place but don't.

I found this very confusing as well. I would suggest the time might be right for having a philosophy (I'm shying away from the word "Rule") about this and strive for greater consistency.

microdev1 · 2021-02-18T02:58:02Z

A follow-up with the current status :-

SPI & I2C might have bugs.
Currently I am focusing on getting things up and running which were stuck due to idf@v4.2.
I suggest creating a new branch for the IDF update. That way it would be easier for folks to collaborate on the update work.

skieast · 2021-02-18T03:35:03Z

Yes. Good idea. I think my fixes to i2c from your branch have that working. No testing on SPI yet. Like you I expect bugs. But there's the fun. I'm currently looking (again) at i2c and wifi crashing. Wifi is not trivial. Lol.

askpatrickw · 2021-02-18T06:22:02Z

@microdev1 and @skieast if you want to try ... I added you as contributors to my fork.
Scott suggested you could add my fork as a remote git remote add askpatrickw https://github.com/askpatrickw/circuitpython.git and then merge into my branch which is connected to this PR. At least I think so... ;-)

Whatever is easiest though.. I'm not concerned about it coming from my repo or this PR.

skieast · 2021-02-18T13:35:47Z

@microdev1 and @skieast if you want to try ... I added you as contributors to my fork.
Scott suggested you could add my fork as a remote git remote add askpatrickw https://github.com/askpatrickw/circuitpython.git and then merge into my branch which is connected to this PR. At least I think so... ;-)

Whatever is easiest though.. I'm not concerned about it coming from my repo or this PR.

Always like to learn new things. I will look at merging my i2c changes. Will have to clean up a bit first as I have some additional boards defined for my use. Maybe during Scott's Deep Dive today. :)

askpatrickw · 2021-03-03T01:31:47Z

@skieast and @microdev1 , I've been away from CP for a few days. How is the IDF update looking? Is there anything I can help with?

skieast · 2021-03-03T04:37:47Z

@skieast and @microdev1 , I've been away from CP for a few days. How is the IDF update looking? Is there anything I can help with?

I too took a little break after running into a wall with the I2C and WIFI issues. I've started again and will probably ignore the wifi for a bit.

askpatrickw · 2021-03-03T07:11:59Z

One thing I didn't quite understand. Are the i2c and WiFi issues in the current IDF version in CP?

skieast · 2021-03-03T13:21:07Z

Yes. I'm not sure when the problem first appeared but it's defnitey there with 4.2. I was hoping that 4.3 would fix or at least change the issue. As I said I'm going to get i2c working then see if spi works. Those were the two areas where compile errors appeared that required changes. Then look at wifi.

askpatrickw · 2021-03-03T21:13:55Z

If 4.3 doesn't introduce any new problems, does it makes sense to Merge 4.3 and work on issues not related to IDF 4.3 separately?

microdev1 · 2021-03-03T21:19:05Z

Thanks! @askpatrickw and @skieast for looking into this.
We can deal with issues not related 4.3 later. So far I have only found i2c and spi to be a concern.

askpatrickw · 2021-03-04T00:32:09Z

@microdev1 Just to make sure I'm following. The i2c and spi issues are the same ones as before or new ones?

If they are not new, maybe you should make a PR from you branch where you had everything building and we can close this.

skieast · 2021-03-04T01:11:02Z

There were changes to the IDF with 4.3 that require changes to i2c and spi. I2C is mostly done and seems to work as before. There is an issue with soft reboot / or wifi. You get to pick which one works. I am starting to look at SPI now. So basically should be the same as before once everything works. SPI had a lot of changes I think so I am hoping whatever microdev did works out of the box. :)

askpatrickw · 2021-03-04T03:57:26Z

Thanks @skieast !

microdev1 · 2021-03-04T04:34:28Z

I am hoping whatever microdev did works out of the box. :)

It should work, shouldn't it. 🤞
I think only spi_hal_cal_clock_conf() has a major change.

skieast · 2021-03-04T15:17:55Z

Lol. Crashed when doing the spi construct. I'm on the ski hill for a few hours and will dig in later. I have an idea what it is. Probably wrong but it's an idea.

askpatrickw · 2021-03-09T08:37:58Z

I had a great chat with @UnexpectedMaker about this update to the IDF, he's going to post more detailed notes, but a couple of things:

1.The paths updates I provided (now ~ a month old) are incorrect in many instances, wherever possible the path should have an esp32s2 folder in the path.
2. In order to support S3, ESP32 as well as the S2, from the same port, the build system will need to have a way to indicate the target board. @tannewt should advise on that, and we should probably break that out as a separate issue. q: Maybe this can go in the board definition?
3. When the second item is worked on, the conditional paths should also be fixed so either the esp32s2 folder is conditionally esp32s3, or esp32 or that there are conditional sections for each target micro-controller's includes

Seon is going to provide a sample of the path changes and if my day allows, I'll give a swing at those changes tomorrow.

@microdev1 @skieast, I'm also happy to bow out if someone else has time and would like to step-in.

Keep in mind, this also fixes building on ~~the~~ my M1 Mac Mini...

microdev1 · 2021-03-09T08:48:23Z

@askpatrickw Thanks for insight on this. Can you please re-post this is in #4363. I think we should continue the conversation there.

askpatrickw · 2021-03-09T08:50:31Z

done.

microdev1 · 2021-03-10T04:57:26Z

@askpatrickw I believe these changes should have there own PR after this is merged.
You can simply pull from my branch to incorporate my commit here. That should also fix the merge conflict and build issues.

microdev1 · 2021-03-10T06:33:10Z

I have updated the PR as discussed on discord.

askpatrickw

I reviewed the changes to the INC in the Makefile and the paths updates in the .c and .h files, they all make sense.
I also built and flashed this successfully to a feathers2 pre-release and ran a program that uses a BME680 over i2c.
Also built and flashed a kaluga and tested an Ultimate GPS over UART which worked as well.

.gitmodules

microdev1 · 2021-03-12T07:47:15Z

Converting this to draft for now as SPI still crashes. Also, I2C will need a re-visit once #4387 is merged.

askpatrickw · 2021-03-27T07:44:28Z

#4387 is merged now what should we verify\fix next ?

askpatrickw · 2021-06-23T02:42:22Z

ESP-IDF v4.3 released

https://github.com/espressif/esp-idf/releases/tag/v4.3

microdev1 · 2021-06-23T13:02:42Z

Finally... this is almost ready to merge. I re-wrote the SPI module, now it utilizes the idf's spi_master driver. I haven't tested the new module completely and some bugs might still be present, it would be really helpful if folks can give it a shot. :)

tannewt · 2021-06-23T17:46:04Z

Thanks @microdev1 ! The code looks good to me. Unfortunately, it looks like Kaluga 1.3 ran out of space in one language.

Could others chime in that they've tested this too? Thanks!

anecdata · 2021-06-23T18:38:09Z

ST7789 SPI displayio and other features seem to be working fine.

But I'm seeing issues with this artifact relative to 7.0.0-alpha.3...

Ejecting CIRCUITPY and restarting on some uncaught exceptions (perhaps only after some feature is invoked):

KeyboardInterrupt: 
$

Also ejecting CIRCUITPY and restarting on trying to lock I2C:

Adafruit CircuitPython 7.0.0-alpha.3-99-gcc7d59c3a on 2021-06-23; FeatherS2 with ESP32S2
>>> import board
>>> i2c = board.I2C()
>>> 
>>> while not i2c.try_lock():
...     pass
...     
...     
... 
$

microdev1 · 2021-06-24T06:13:08Z

I have fixed the Kaluga 1.3 space and i2c.try_lock() issues. I haven't noticed any uncaught exceptions yet.

anecdata · 2021-06-24T12:39:22Z

Confirmed i2c.try_lock() is good now.

Try this code to see the exception crash:

import board
import time

time.sleep(5)

import adafruit_dotstar
# status_light = adafruit_dotstar.DotStar(board.APA102_SCK, board.APA102_MOSI, 1, brightness=0.1)

while True:
    print("", time.monotonic(), end="\r")
    time.sleep(1)

control-c within the loop works fine unless the DotStar line is uncommented

- update idf submodule to release/v4.3 - finish todo tasks held due to idf@v4.2 - update SPI & I2C to make them v4.3 compatible

microdev1 · 2021-06-24T17:47:13Z

@anecdata Thanks for the thorough testing, the exception crash is now resolved.

tannewt

Thank you for updating the IDF!

tannewt mentioned this pull request Mar 8, 2021

Add ESP32-S3 support #4363

Closed

askpatrickw commented Mar 10, 2021

View reviewed changes

.gitmodules Outdated Show resolved Hide resolved

microdev1 marked this pull request as draft March 12, 2021 07:46

hierophect added the espressif applies to multiple Espressif chips label Mar 27, 2021

microdev1 marked this pull request as ready for review June 23, 2021 12:44

microdev1 requested a review from tannewt June 23, 2021 12:44

microdev1 changed the title ~~Update idf~~ Update esp-idf to v4.3 Jun 23, 2021

microdev1 linked an issue Jun 24, 2021 that may be closed by this pull request

ESP-IDF Update to unblock Mac M1 Builds #4182

Closed

update idf to v4.3

e0eecd2

- update idf submodule to release/v4.3 - finish todo tasks held due to idf@v4.2 - update SPI & I2C to make them v4.3 compatible

tannewt approved these changes Jun 25, 2021

View reviewed changes

tannewt merged commit 5a4a803 into adafruit:main Jun 25, 2021

jepler mentioned this pull request Jun 30, 2021

Kaluga not connecting via USB (since esp-idf update?) #4951

Closed

jfurcean mentioned this pull request Jul 2, 2021

ESP32 hard crash on i2c display #4962

Closed

Update esp-idf to v4.3 #4195

Update esp-idf to v4.3 #4195

Conversation

askpatrickw commented Feb 13, 2021

skieast commented Feb 13, 2021

askpatrickw commented Feb 13, 2021

skieast commented Feb 14, 2021

askpatrickw commented Feb 14, 2021 • edited

skieast commented Feb 14, 2021

microdev1 commented Feb 14, 2021

skieast commented Feb 14, 2021

askpatrickw commented Feb 14, 2021

hierophect commented Feb 16, 2021 • edited

skieast commented Feb 16, 2021

tannewt commented Feb 16, 2021

askpatrickw commented Feb 17, 2021

microdev1 commented Feb 18, 2021

skieast commented Feb 18, 2021

askpatrickw commented Feb 18, 2021

skieast commented Feb 18, 2021

askpatrickw commented Mar 3, 2021

skieast commented Mar 3, 2021

askpatrickw commented Mar 3, 2021

skieast commented Mar 3, 2021

askpatrickw commented Mar 3, 2021

microdev1 commented Mar 3, 2021

askpatrickw commented Mar 4, 2021

skieast commented Mar 4, 2021

askpatrickw commented Mar 4, 2021

microdev1 commented Mar 4, 2021

skieast commented Mar 4, 2021

askpatrickw commented Mar 9, 2021 • edited

microdev1 commented Mar 9, 2021

askpatrickw commented Mar 9, 2021

microdev1 commented Mar 10, 2021

microdev1 commented Mar 10, 2021

askpatrickw left a comment • edited

Choose a reason for hiding this comment

microdev1 commented Mar 12, 2021

askpatrickw commented Mar 27, 2021

askpatrickw commented Jun 23, 2021

microdev1 commented Jun 23, 2021

tannewt commented Jun 23, 2021

anecdata commented Jun 23, 2021 • edited

microdev1 commented Jun 24, 2021

anecdata commented Jun 24, 2021 • edited

microdev1 commented Jun 24, 2021

tannewt left a comment

Choose a reason for hiding this comment

askpatrickw commented Feb 14, 2021 •

edited

hierophect commented Feb 16, 2021 •

edited

askpatrickw commented Mar 9, 2021 •

edited

askpatrickw left a comment •

edited

anecdata commented Jun 23, 2021 •

edited

anecdata commented Jun 24, 2021 •

edited