Skip to content
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

cpu/esp8266: complete reimplementation based on ESP8266 RTOS SDK #11108

Merged
merged 20 commits into from
Nov 18, 2019

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Mar 5, 2019

Contribution description

This PR is a complete reimplementation of the RIOT port for the ESP8266 platform. Advantages of the reimplementation are:

  • requires less memory
  • provides much more heap space
  • doesn't need any hacks to keep SDK's internal multitask system alive
  • large parts are source code compatible with ESP32
  • code deduplication will be available
  • even more stability of WiFi interface

Although it is a development revision, it is already completed, works very stably and is only one step away from merge. It simply requires some cleanups before it can be merged.

The former implementation was realized either as a bare metal implementation without WiFi or using the ESP8266 NONOS SDK to enable WiFi functionality. Unfortunatly, the SDK version had different disadvantages and a lot of problems:

  • It required a lot of RAM.
  • It included its own very old version of lwIP.
  • It included its own multitask system.
  • It was closed source.

With version 3.0 of the RTOS SDK, Espressif started in last fall to release a SDK based on FreeRTOS in source code which is almost source code compatible with the ESP-IDF for ESP32. As with ESP-IDF, the RTOS SDK can be used to port RIOT to ESP8266, provided there is a small FreeRTOS adaptation layer which maps the FreeRTOS functions used by SDK libraries to according RIOT functions.

The source code compatibility of the ESP8266 RTOS SDK and the ESP-IDF for ESP32 allows to reuse a lot of code for ESP8266 and ESP32. For example, the same code can be used for context handling, interrupt handling, WiFi networking and peripheral interfaces like I2C, SPI and UART interfaces. Most of the files of this reimplementation are already to be merged with ESP32 source code.

As a first evolution step, the reimplementation for ESP8266 resides in parallel to the implementation for ESP32. The code for ESP8266 and ESP32 will be merged later step by step.

Status

Everything that was working with the old implementation is working with the reimplementation and already tested.

  • periph/adc
  • periph/cpuid
  • periph/flash
  • periph/gpio
  • periph/hrwng
  • periph/i2c
  • periph/pm
  • periph/pwm
  • periph/spi
  • periph/timer
  • periph/uart
  • netdev/esp_now
  • netdev/esp_wifi

ToDos

  • toolchain changes
  • documentation
  • WDT handling
  • buffering for esp_wifi
  • software timer (if possible at all)
  • esp_gdbstub adaption for debugging capabilities

Possible improvements

  • improvements of panic handler (backtrace)

Issues/PRs references

This PR depends on PRs #9917, #10883, #10929, #10953, #10981, #10982, #11148, #12505, #12510 , RIOT-OS/riotdocker#93

@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Impact: major The PR changes a significant part of the code base. It should be reviewed carefully State: waiting for other PR State: The PR requires another PR to be merged first Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Mar 5, 2019
@gschorcht gschorcht force-pushed the cpu/esp8266/esp-idf/pr branch from 7d7936a to 7db70f4 Compare March 14, 2019 14:43
@gschorcht gschorcht removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Mar 20, 2019
@gschorcht gschorcht requested a review from smlng April 12, 2019 08:33
@gschorcht
Copy link
Contributor Author

@smlng @MrKevinWeiss When the current semester is over in a few weeks, I will have time again to continue this work. I think the reimplementation is an important step towards better quality. Especially with "esp_wifi" being used, the stability of the reimplementation with the new RTOS SDK seems to be much better than with the old SDK.

Do you think that there will be a chance to have the code reviewed by you?

@gschorcht
Copy link
Contributor Author

I hope I have some time over the next few days to rebase the PR to resolve the conflicts.

@smlng
Copy link
Member

smlng commented Jun 5, 2019

sure, just give us another ping when you made the rebase and you think this is ready to be reviewed and tested. I will find time for that ...

@gschorcht
Copy link
Contributor Author

sure, just give us another ping when you made the rebase and you think this is ready to be reviewed and tested. I will find time for that ...

Great, I will ping again.

@gschorcht gschorcht force-pushed the cpu/esp8266/esp-idf/pr branch 2 times, most recently from b8a7357 to 1854809 Compare September 5, 2019 14:18
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 5, 2019
@gschorcht
Copy link
Contributor Author

sure, just give us another ping when you made the rebase and you think this is ready to be reviewed and tested. I will find time for that ...

Ping @smlng 😉 After 2 weeks of hard work I'm really proud to inform that I have finished the rebase/rework of this PR with a lot of improvements and a lot of extensions.

In fact, the reimplementation has been finished. It is working very stable and the WiFi interface is even more stable than before. Furthermore, almost all the files that can be merged in the future with the ESP32 code are ready to be merged. You can see this when you search for #ifdef MCU_ESP32 in the source code. And, all these files have already been tested with ESP8266 as well as ESP32. That is, their merge will become pretty easy.

The only open task that remain might be an implementation of newlibs _lock_* functions. On the other hand, all tests worked without any problems with newlib's dummy _lock_* functions. So I'm not sure whether it is really usefull to implement them. I have seen no RIOT platform that implements newlib's lock functions. Even more, implementing _lock_* function based on mutexes might lead to problems when locking newlib functions are called from interrupt context.

Anyway, from my point of view, the reimplementation is ready and it is a big step forward. I would be really grateful if you could find some time to review it berfore it becomes toooooo old and has to be rebased with a lot of effort again.

@gschorcht
Copy link
Contributor Author

gschorcht commented Sep 5, 2019

@smlng Since riotdocker does not include the toolchain til now, you might want use the special riotdocker image schorcht/riotbuild_esp8266_rtos_sdk which contains the whole toolchain with the exception of the esptool.py. Otherwise, you could install the toolchain as following:

mkdir -p ~/esp
cd ~/esp
git clone https://github.com/gschorcht/xtensa-lx106-elf
git clone https://github.com/gschorcht/RIOT-Xtensa-ESP8266-RTOS-SDK.git ESP8266_RTOS_SDK
cd ESP8266_RTOS_SDK
git checkout -q f074414c0705715a44b8e59d53b03d90b7630382
cd ~/esp
git clone https://github.com/gschorcht/esptool-esp8266-rtos-sdk-v3
chmod +x ~/esp/esptool-esp8266-rtos-sdk-v3/esptool.py
export PATH=~/esp/esptool-esp8266-rtos-sdk-v3:~/esp/xtensa-lx106-elf/bin:$PATH

Please note that the esptool.py required for ESP82666 RTOS SDK differs from the esptool.py you have probably installed on your host system.

@gschorcht gschorcht removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 5, 2019
@gschorcht
Copy link
Contributor Author

Ping @smlng. I know, there are a lot of things to do for release 2019.10. I just wanted to asked you kindly, whether it will become possible for you to start the review of this PR in next weeks. As I said, I spent 2 weeks to rebase the PR and to resolve the all the conflicts and it is starting to have conflicts again 😟

In vendor startup code, initialization function were called as parameters of assert statement. With DEVELHELP, they are not called since the assert macro does nothing.
High priority thread for the WiFi interface are only created at startup when the WiFi interface is used.
The modified version esptool.py from RTOS SDK that is required for flashing an image, is now placed in `dist/tools/esptool.py` and used directly from there. The advantage is that `esptool.py` hasn't to be installed explicitly anymore. Having RIOT is enough. The documentation is adapted accordingly. The oly prerequisite is that python and the pyserial module are installed.
@gschorcht gschorcht force-pushed the cpu/esp8266/esp-idf/pr branch from 5b002b8 to 555a704 Compare November 14, 2019 13:00
@gschorcht
Copy link
Contributor Author

They have the same results but they execute faster. It is an improvement but also it would be good to get this PR in instead of adding more and more. The reset tool probably could have been a separate PR.

Of course, you are right. It was just a try to fix the remaining problems.

At the moment I am waiting for a suggestion from @kaspar030 how to deal with the two basic problems (the newlib locking functions and the watchdog timer). Alternatively, I could disable both.

@MrKevinWeiss Since there was no suggestion on how to deal with these problems 😟 (even though I believe that at least the first two are more general problems and not specific problems of esp8266), I worked around them by

  1. disabling lock functions for the newlib,
  2. disabling the watchdog timer, and
  3. changing optimization option to -O2

and squashed the PR. With these changes all automatic tests work except the gnrc_* tests that require root privileges. They have to be started intentionally with sudo make test to get them to work.

From point of view, the PR is in a state where it could be merged.

@gschorcht
Copy link
Contributor Author

gschorcht commented Nov 14, 2019

Let me know if I can help further.

@kb2ma I have it running now for two hours. It seems still to work, independent on whether I'm away from the computer on which the server is running or doing something with it. I have the same configuration as you, my computer is connected via Ethernet while esp8266 is connected using the WiFi in the same network.

Even though I can't say whether the packets are correct (have not used COAP til now), I have every about 300 seconds two messages

esp8266                                                                      server
   | ------------------- CoAP POST [Uri-Path: /rd/mFbcDmwXnV] ----------------> |
   | <----------- CoAP ACK, 2.04 Changed [Uri-Path: /rd/mFbcDmwXnV] ----------- |

I have activated Observe for the whole device.

@kb2ma
Copy link
Member

kb2ma commented Nov 14, 2019

Those results sound like what is expected. Thanks for the verification. I will experiment with components on my local setup. Certainly I don't see a reason to hold up this PR.

@gschorcht
Copy link
Contributor Author

@kaspar030 BTW, with the new reset tool in commit 555a704 it will be possible to enable esp8266 for CI testing using any ESP8266 NodeMCU. All automatic tests are working with this reset tool with the exception of the gnrc_* tests that require root privileges.

@gschorcht
Copy link
Contributor Author

gschorcht commented Nov 17, 2019

@MrKevinWeiss Since there was no suggestion on how to deal with these problems (even though I believe that at least the first two are more general problems and not specific problems of esp8266), I worked around them by

  1. disabling lock functions for the newlib,
  2. disabling the watchdog timer, and
  3. changing optimization option to -O2

and squashed the PR. With these changes all automatic tests work except the gnrc_* tests that require root privileges. They have to be started intentionally with sudo make test to get them to work.

From point of view, the PR is in a state where it could be merged.

Ping @MrKevinWeiss Do you think that you could merge the PR with these last changes?

@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 17, 2019
@MrKevinWeiss
Copy link
Contributor

Yup, let me just rerun murdock then we can merge!

@MrKevinWeiss MrKevinWeiss merged commit be39169 into RIOT-OS:master Nov 18, 2019
@MrKevinWeiss
Copy link
Contributor

Very nice @gschorcht! Thanks for all the hard work.

@kb2ma kb2ma added this to the Release 2020.01 milestone Nov 18, 2019
@gschorcht
Copy link
Contributor Author

gschorcht commented Nov 18, 2019

@MrKevinWeiss Many thanks again for your support and all your work you have spent into reviewing and testing this mega PR.

Now, I will start to deduplicate several parts of the ESP8266 and ESP32 code step by step.

@gschorcht gschorcht deleted the cpu/esp8266/esp-idf/pr branch November 18, 2019 15:58
@MrKevinWeiss
Copy link
Contributor

Sounds good!

gschorcht added a commit to gschorcht/RIOT-Xtensa-ESP that referenced this pull request Jan 4, 2020
The new ESP8266 port in PR RIOT-OS#11108 requires a different tool chain than the previous implementation. The version check must be updated accordingly.
gschorcht added a commit to gschorcht/RIOT-Xtensa-ESP that referenced this pull request Jan 4, 2020
The new ESP8266 port in PR RIOT-OS#11108 requires a different tool chain than the previous implementation. The version check must be updated accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: ESP Platform: This PR/issue effects ESP-based platforms Process: needs >1 ACK Integration Process: This PR requires more than one ACK Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants