search

esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
699 stars 191 forks source link

Provide a more robust solution for getting the current time #1944

Open Dominaezzz opened 1 month ago

Dominaezzz commented 1 month ago

The current implementations for getting the time (since boot?) are:

These implementations unfortunately have a couple issues:

I'm not sure what the solution is yet besides simply removing these APIs, but if getting the time without direct access to a timer peripheral is that desirable, then the hal user needs to explicitly create a timer driver and give up access to one of its counters somehow, something along the lines of esp_hal_embassy::init.

Perhaps this feature doesn't belong directly in hals and should be exposed by 3rd party libraries similar to embassy_time_driver and critical_section.

Or maybe this could be like the esp_hal_embassy feature where SoftwareInterrupt<3> is inaccessible. Though this doesn't solve the enable/reset problem.

bjoernQ commented 1 month ago

There are definitely things to improve here.

... if getting the time without direct access to a timer peripheral is that desirable ...

I think it is

They do so assuming the peripherals have been enabled and pulled out of reset.

The HAL should/could (and currently does?) guarantee that (ok - we don't enable them on boot if they are not, yet)

They also assume that the user won't reset them (again) by recreating the driver

I guess that is something the drivers need to take care of. If we really need to reset those, the driver should take care to save and restore the counters

They also assume that the user won't change the counter values in the peripherals

I think we don't offer an API for that? - if a user uses direct register manipulation for that, all our assumptions are invalid, I guess (in general true for all drivers)

Dominaezzz commented 1 month ago

They also assume that the user won't change the counter values in the peripherals

I think we don't offer an API for that? - if a user uses direct register manipulation for that, all our assumptions are invalid, I guess (in general true for all drivers)

I guess this is the main issue. As far as the user is concerned, if they have access to the TIMG0 or SYSTIMER peripheral via peripherals.TIMG0 or peripherals.SYSTIMER, then they have full ownership of the peripheral and are free to tinker with registers. The hal breaks this rule in these APIs to get the current time.

... if getting the time without direct access to a timer peripheral is that desirable ...

I think it is

Yeah I figured. Short term, it's probably best to have a esp_hal::time::init(....) that just takes an erased timer. This solves all the problems I mentioned.

Dominaezzz commented 6 days ago

In addition to the global time setup, it'd be a good idea to setup an interrupt to keep track of the timer wrapping around as well (Both SYSTIMER and TIMG can do this at least), the we can have a current time that only increments forward and doesn't wrap around.

MabezDev commented 4 days ago

it'd be a good idea to setup an interrupt to keep track of the timer wrapping around as well (Both SYSTIMER and TIMG can do this at least)

Unless I'm misunderstanding what part is wrapping around here, the smallest wrap around possible is ~7years, the highest being 36558 years on the esp32 - is this something real world applications really have to consider?

Dominaezzz commented 4 days ago

it'd be a good idea to setup an interrupt to keep track of the timer wrapping around as well (Both SYSTIMER and TIMG can do this at least)

Unless I'm misunderstanding what part is wrapping around here, the smallest wrap around possible is ~7years, the highest being 36558 years on the esp32 - is this something real world applications really have to consider?

Ah, I was under the impression that the wrap around number was much smaller for some reason. 36558 years is plenty.