minor issues in core_esp8266_waveform.h

[konohh]

• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).

Platform

• Hardware: [ESP-12]
• Core Version: [latest git]
• Development Env: [Arduino IDE]
• Operating System: [Windows]

Settings in IDE

• Module: [Generic ESP8266 Module|Wemos D1 mini r2]

Problem Description

core_esp8266_waveform.h

For my recent project I needed a (relatively precise) timer controlled trigger for my ISR function and I found it in the above mentioned part of the ESP core. I got it eventually working, nevertheless there are two minor issues I would like to address.

1. It is documented in the "core_esp8266_waveform.h" that the CB function (set by "setTimer1Callback()") should return a value in microseconds (to set the time for the next call). This is not correct, the CB function must return the clock cycles to the next call. (Which as such is fine and absolutely correct).
2. As the CB function is called in an Interrupt, the CB function has to be assigned with the ICACHE_RAM_ATTR attribute. To calculate the clock cycles to be returned by the CB function it may be essential to get the actual clock cycles. As per documentation this shall be done by calling "ESP.getClockCycles()". As far as I could figure it out, this function does not carry the ICACHE_RAM_ATTR attribute and therefore using it in an interrupt, the code crashes. Surely because of that issue, in the "core_esp8266_waveform.cpp" an inline function "GetCycleCount()" with the attribute ICACHE_RAM_ATTR is implemented and used. This "GetCycleCount()" function should be documented, prototyped and made available in the "core_esp8266_waveform.h" file.

Please bear with me, I'm not a software guy. If some of the above terms and definitions are not the correct ones… Sorry.

[Tech-TX]

Pull request #7231 will be merged sometime soon, you may want to see if it already addresses your concern.

[earlephilhower]

@konohh, with the latest merges we've done pretty big changes to the waveform code. Can you check if they address your issues and report back? Thx!

[dok-net]

1. It is documented in the "core_esp8266_waveform.h" that the CB function (set by "setTimer1Callback()") should return a value in microseconds (to set the time for the next call). This is not correct, the CB function must return the clock cycles to the next call. (Which as such is fine and absolutely correct).

If it's documented that way, and the remainer of the waveform API uses microseconds, not cycles (disregard latest changes), it's backward, the implementation was wrong. @earlephilhower The two "saveurs" of waveform now differ in this regard, because I changed the implementation to match the spec/docs. I think it has to be in microseconds for consistency and avoiding forcing CPU frequency issue handling into user sketch code beyond absolute necessity.

2. As the CB function is called in an Interrupt, the CB function has to be assigned with the ICACHE_RAM_ATTR attribute. To calculate the clock cycles to be returned by the CB function it may be essential to get the actual clock cycles. As per documentation this shall be done by calling "ESP.getClockCycles()". As far as I could figure it out, this function does not carry the ICACHE_RAM_ATTR attribute and therefore using it in an interrupt, the code crashes. Surely because of that issue, in the "core_esp8266_waveform.cpp" an inline function "GetCycleCount()" with the attribute ICACHE_RAM_ATTR is implemented and used. This "GetCycleCount()" function should be documented, prototyped and made available in the "core_esp8266_waveform.h" file.

Don't worry, you haven't dug deep enough - it both works in existing code and you would find that due to forced inlining of assembly there's no need for the attribute.

[devyte]

The time to set can't be in us, the us resolution isn't fine enough. That was found during the exhaustive testing of both #7022 and #7231: it caused linearity problems, staircasing, drift, and other problems, especially at higher frequency. This is the reason why the internals were changed to (cpu) cycles, and the original user-facing api was refactored as forwarders with conversion from us to cycles.
I guess updating the docs is still pending.

[devyte]

Closed via #7717