earlephilhower / arduino-pico

Raspberry Pi Pico Arduino core, for all RP2040 and RP2350 boards
GNU Lesser General Public License v2.1
2.03k stars 422 forks source link

rp2040 Ticker Library #1376

Closed hreintke closed 1 year ago

hreintke commented 1 year ago

Hi,

I made a Ticker library using the alarm function. The interface is similar to the Ticker libraries from esp8266/esp32.

I can PR that into your repo if you are interested.

Will add/alter functionality if you require so.

https://github.com/hreintke/ArduinoLibraries/tree/master/RP2040_Ticker

earlephilhower commented 1 year ago

Neat, thanks. I like how simple it is to implement since the Pico SDK has such a nice alarm interface.

Only thing I see that might be missing is, would you be able to add an example or two? That way the code would be compiled on every CI run (ensuring a PR doesn't break this functionality...or at least doesn't break compilation) and let folks see how to use it.

I suppose a git submodule would be fine if you intend on doing lots of updates, or you could add the files to the repo directly in your PR.

hreintke commented 1 year ago

No, there will not be a lot of updates, not much functionality to add.

Only thing I think to add is the possibility to use a custom alarm pool. Current version is always using the default alarm pool, using CPU 0.

Will add an additional constructor and use the alarm_pool_ version of the sdk calls. Examples need to added, and I have to check on private/public for variables and functions.

Is there an Astyle you want the code to apply to ?

earlephilhower commented 1 year ago

No rush, take your time.

The restyler is in tests/restyle.sh. The general contrib docs are here: https://arduino-pico.readthedocs.io/en/latest/contrib.html

sstaub commented 1 year ago

I see a conflict with my library (and maybe others existing libraries) https://github.com/sstaub/Ticker I solved it with writing a renamed library https://github.com/sstaub/TickTwo but I'm not happy with it. @hreintke there is no README document and license file, also there are no basic comments in the code. You should learn to write libraries before making a PR.

hreintke commented 1 year ago

@sstaub No need to use that kind of tone. I do know, and mentioned it above, that there need to be updates in order to make it a complete library. I am working on that now. That is also why there is no PR yet, just an issue for WIP and communication.

Naming conflicts can be solved with the use of namespaces.

earlephilhower commented 1 year ago

I'm not seeing an issue with naming. There are multiple Ticker libraries out there, so this isn't a unique problem.

The first one I thought of was the ESP8266 core-included one by Ivan G: https://github.com/esp8266/Arduino/tree/master/libraries/Ticker

Since this hooks directly into the RP2040 Pico-SDK, using a architectures=rp2040 should limit its use to this core, and the fact that it's using OS-level timer callbacks means it's simpler to use (like the ESP8266 one) where you set a ticker and don't have to worry about calling a tick() or update() every now and then in your code. We've gone that way for the LWIP and BT stacks as well and it's just much nicer for the end users, IMHO.

That said, because it's doing it on the alarm then the callback code will execute in ISR context so it can get hairy (not a good idea to touch flash, do anything taking long, do any delay(), etc.). That's the same issue as the ESP8266 one I'm familiar with.

--edit--

Actually, looks like the ESP32 has a good integrated Ticker library to start with as well. Might want to look at it and see if it's trivial to insert the alarm calls in place of the Espressif IDK calls: https://github.com/espressif/arduino-esp32/tree/master/libraries/Ticker

hreintke commented 1 year ago

I have a version now which includes examples and working. Still need to check on thread/ISR safety.

I know the ESP32 implementation and will check the "alarm replacement" possibility. The ESP32 version does not include functional callbacks, the one I am working on does. Is it OK if I add that .