Open rasmuskleist opened 1 year ago
1) Probably yes. The current RCC implementation assumes there is only one enable bit for each peripheral.
2) Have a look at std::chrono::year_month_weekday to see if that's usable. std::chrono
would be preferable to the C time API. I'd like to have a standard API for the RTC independent of the hardware representation. I'm curious what using the standard calendar APIs does to the flash size. There are big things it could accidentally pull in, like the timezone data, if we are unlucky. You'd have to try to see if it's practical or not.
3) Yes, that's a good idea. You can put them into the math:utils
module and ideally add a unit test in modm/test/modm/math/utils
.
I have rewritten the lbuild script to allow multiple enable bits. For the G4 family it seems only to be the RTC that has two enable bits. Parts of my solution might be somewhat hacky or what do you think? Specifically, I was envisioning a more elegant solution of writing the return statements in isEnabled, but I could not find a similar use case of lbuild. Do you have any suggestions or should I just leave it as is?
It occured to me that RCC_BDCR_RTCEN
is being set in both Rcc::enableRealTimeClock
and in Rcc::enable
with the upstream build script. I have thus changed the build script to generate code that sets RCC_APB1ENR1_RTCAPBEN
in the Rcc::enable
function. I have done this by not overwriting entries in the rcc_enable
dict. I am unware if this causese unwanted behaviour for other devices than STM32G4, but I do find it vulnerable that the code generated is dependent on the sorting of rcc_map
or what do you think?
I am also debating if the Rtc
class should take the asynchronous and synchronous prescalars as template paramters. This will allow me to define using resolution = std::ratio<1, Synchronous + 1>
and using duration = std::duration<int32_t, resolution>
inside Rtc
, which will be convenient for conversions for example in synchronize
and simplify the code in other places. I also think it will possibly be able to statically assert that the RTC clock frequency is less than the APB1 clock frequency and use if consexpr
in other places involving the RTC clock frequency. What do you think?
I pressume that the ideal interface for the Rtc would be similar to std::chrono::steady_clock
, basically that it implements now
which returns std::chrono::time_point
with epoch being the zero of the RTC? Here std::time_point
could use std::chrono::seconds
, std::chrono::milliseconds
or resolution
from above, each presenting their own problems. For my own use case, I need better precision than seconds, but with a uint32_t
the milliseconds will wrap around in 49.7 days. Meanwhile, the seconds will wrap around in 136 years, which is more than enough for the RTC. The resolution
will have the best attainable precision and depending on the choice of synchronous prescaler an intermediate range. Specifically, if PREDIV_S = 0xFF
(default) the range is approximately half a year. What are your thoughts on this?
For initialization something I think std::chrono::year_month_day
could be suiteable suitable, without knowning the exact details. Presumably, time since midnight would then be passed on with std::chrono::seconds
?
Thanks for the nice comments! I have tried to resolve most of the issues and will solve the remaining hopefully this week. In resolving these issues I have encountered the following problems that I would like your take on:
initialize
a third template parameter namely RTCCLK
. This can vary significantly depending on the clock source used for the RTC. Typically a 32.768 kHz LSE, will be used, but HSE can also be used. Therefore I think the most flexible way of getting RTCCLK
into initialize
is to pass it manually. I choose to pass it as a template parameter, so I can statically assert that it is not larger than the APB1 frequency.SysTickTimer
because it has two prescalers. These should be chosen to generate ck_spre
of 1 Hz (usually). Hence there can be multiple good choices of prescalers dependning on the objective. I suggest the objective will be to choose PREDIV_A
as large as possible (default) and choose PREDIV_S
accordingly such that PREDIV_A * PREDIV_S = RTCCLK
. This decreases the RTC dynamic consumption as much as possible. However, this is a small optimization problem, which may be difficult to solve. Do you have other suggestions to the choice of prescalers?ck_spre
cycle similar to SysTickTime
.ck_apre
and ck_spre
are re-initialized on startup. However, in that case the calender and prescalers will most likely not have to be initialized again. For my own application this is mostly in the unlikely event of a short power loss. Therefore it may make sense to implement multiple initialize functions for example setDateTime
? However, this is not a major concern from my part at least with this iteration.now
which returns a time_point
. However, the value of the time_point
should be calculated from the value of RTC_SSR
, RTC_TR
and RTC_DR
?Thanks in advance for your help!
Currently, I have also given initialize a third template parameter namely RTCCLK.
No, that informatio should come from SystemClock, which should contain the full clock configuration.
There should be a member SystemClock::Rtcclk
with the right frequency or whatever is the most canonical form.
If the HSE is used, then the SystemClock needs to reflect that (and SystemClock::enable()
needs to configure that).
Do you have other suggestions to the choice of prescalers?
Prescaler A is only 7-bit so you can linearly iterate 1-128 and find a suitable prescaler S that fits the tolerance. Maybe a prescaler s ≥1000 ticks so that you get millisecond resolution? You can do it at compile time with constexpr, then it's easy to code and costs almost nothing in compile time and you can unit test it separately.
Perhaps add the algorithm to the Prescaler
class, it may be used by other RTCs or at least be similar.
Perhaps you should pass a frequency parameter so that people can specify what subsecond frequency they require.
That would probably be the cleanest abstraction, as it also allows for "weird" frequencies to work correctly.
So Rtc::initialize<Board::SystemClock, 1_kHz>();
for the prescaler s ≥1000.
Therefore it is difficult for me to see how you will avoid doing the calender calculations in software.
Ok, that's fine, just may have been nice.
However, in that case the calender and prescalers will most likely not have to be initialized again.
modm does not yet have the concept for brown out detection. Just reinitialize everything.
As you write I should probably only implement now which returns a time_point. However, the value of the time_point should be calculated from the value of RTC_SSR, RTC_TR and RTC_DR
Ugh, I was hoping there'd just be a uint32_t unix timestamp in there.
Maybe there is a good third party library for converting between the unix timestamp since 1970 and RTC?
Otherwise this might not be worth the effort, you might as well use the modm::Clock
then…
This is still work in progress, but I believe that I need a bit of help to finish it up! Currently, I intend to use RTC solely for the for getting timestamps used for logging to a flash chip. This PR should resolve some of the points discussed in #730.
I have left a lot of TODOs in code which I will try to resolve within the near future. However, I would like your input on the following points:
rcc
, the enable peripheral function does not enable the the APB1 peripheral clock for RTC (at least for the G4 family). The function does however enable the RTC withRCC->BDCR |= RCC_BDCR_RTCEN;
. I believe that both should be enabled within this function, or what do you think?std::tm
exists, and I am debating if I should use this or some other standard library struct. I have not implemented the driver withstd::tm
yet because it counts years since 1900 and weekday starting from 0. This does not align with the STM32G4 RTC which only support two digit years and where Monday = 1 and Sunday = 7. Of course, I can make the conversion, but then this would have to be carried out everywhere in the RTC code. What is your opinion on this?math::utils
or somewhere a like? Afterall there are other drivers also converting to and from BCD in modm.Finally, I have not yet devised a novel test for verifying that the
synchronize
function is actually working. From looking at the documentation I suppose it should be working, but my experience is that there is usually, some bug lurking in my untested code. Have you got any idea for such a test? My initial thought was to test if I could (approximately) zero the subsecond counter.