-
Notifications
You must be signed in to change notification settings - Fork 111
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
Update for latest Zephyr version (ESF-149) #113
base: master
Are you sure you want to change the base?
Conversation
Hi @marekmatej, thank you for the fix. LGTM. Will wait for the merges to test it properly. Just a nitpick, can you please change the commit message? We use conventional commits which is checked by CI. Something like @DNedic PTAL. |
zephyr/CMakeLists.txt
Outdated
|
||
zephyr_interface_library_named(esp_flasher) | ||
zephyr_interface_library_named(esp_flasher) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of lines have been just reformatted to remove indentation, can we revert these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, so the order of the statements was a bit strange, and also I didn't like the unnecessary indentation so I fix it according to my taste. However, I can revert all that eye sugar if you don't like it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on CMake formatting, but it would be more consistent with the rest of esp-serial-flasher
to leave it as is, and also make the diffs smaller and git blame more meaningful.
@@ -1,34 +0,0 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the reasoning behind removing the board dt overlay? I'm a bit confused on this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes there are too many boards overlay that are copy and paste stuff. For those cases, there is the socs
folder where you can put all overlays/conf
common on the SoC level. This was the exact case.
Hi @marekmatej , nice work and thanks for helping out! |
Also, I would use the |
Thanks for taking a look @DNedic, this should also fix the small Flash size for the images, that's why I wanted it to be fix, but I agree with the feat. Just so you understand. |
65224a3
to
07f1bbe
Compare
@marekmatej can you please revert the CMakeLists changes? After that, I am going to merge it. I tested it and it works with minor fix not related to this PR. Thank you so much for your help. |
Update Zephyr example for latest Zephyr version. - replace board with the socs specific config - fix warnings Signed-off-by: Marek Matej <marek.matej@espressif.com>
07f1bbe
to
5d380bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
Which version of Zephyr was this tested against? I'm testing against main ( |
Hi @beriberikix, the fix used the upstream version as well, but that was weeks ago. Try to find |
But both files exist in the same directory: https://github.com/espressif/esp-serial-flasher/tree/master/include... |
@beriberikix I am still able to build the zephyr example with this fix. I do not know much about zephyr so I am following the CI build process which is described here. I just use newest releases of all tools and master branch of zephyr. The build should be successful with this. You also need to remove Sorry for inconvenience, these changes will hopefully be merged soon, we are waiting for next zephyr release so we can mark it as supported. Thanks for understanding, I already proposed that tested versions should be stated with all ports. If you have some troubles, let me know. |
Totally understandable! I retried with a clean Zephyr environment and got the same issue. If I explicitly add the
However, linking fails because of undefined references. Ex.
Any ideas? |
Did you clone the this repo and checked out correct branch? If so, you should be able to build the example using following commands:
I am able to build it using these commands. |
Hmm...yes, and just checked again. I didn't pass in the ZEPHYR_EXTRA_MODULES originally by that didn't make a difference. |
Actually, I'm not sure what's the issue with my machine but I was able to build using Docker. Here's the Dockerfile I pulled together:
|
That is interesting. Do not you have exported some variable that messes up with the zephyr build? For example the IDF_PATH? Someone has ESP-IDF export script in .bashrc which might cause some issues. |
This provides multiple fixes to make the Zephyr example work on the ESP32 boards with uart1 available.
Related PRs:
esp_wrover_kit
.