setTime with subSeconds

[charlie1024]

Hello, I'm using recently updated library. The library is very comfortable because I don't have to use additional DS3231.

Anyway, I want to talk about the setTime function.

At now, setTime has two types, the first one is setting HH:MM:SS, and another one sets HH:MM:SS:sss AM/PM. In my opinion, the first one could be changed to setting HH:MM:SS:sss by default. Of course, there's possibility that someone don't want to initialize the sss = 0, and that would be resolved with the code.

This is my suggestion(It's just an example I use).

In STM32RTC.cpp, I slightly changed the previous first one setTime to this(It is strange that newline not appears... but there is not really added, I just brought if(subSeconds < 1000) code from the second one, that's the only difference)

void STM32RTC::setTime(uint8_t hours, uint8_t minutes, uint8_t seconds, uint32_t subSeconds)
...
    syncTime();
    if(subSeconds < 1000) {
      _subSeconds = subSeconds;
    }
...

and in STM32RTC.h,

void setTime(uint8_t hours, uint8_t minutes, uint8_t seconds, uint32_t subSeconds = 1000);

So, the setTime changes the subSeconds only if the arbitrary subSeconds is set, and unless that, subSeconds will be preserved. I tried to implement this because I have to daily-network-updated, very precise clock. I think the setTime can be more generalized with the second setTime, but I'm not an expert to the entire code... Also, in the library it seems that syncTime() must be run before setting the AM/PM so it will involve more edits. If developers think this is reasonable, I would suggest to adding this and the library could be more specific and comfortable.

Thank you.

[fpistm]

Hi @charlie1024, right this sounds reasonable. About syncTime() could you point where you think that then I will check that.

[charlie1024]

Hello, In setTime() there's syncTime() call. In here I'll bring somewhat simpler than setTime(), which is setHours().

void STM32RTC::setHours(uint8_t hours)
{
  if (_configured) {
    syncTime();
    if(hours < 24) {
      _hours = hours;
    }
    RTC_SetTime(_hours, _minutes, _seconds, _subSeconds, (_hoursPeriod == AM)? HOUR_AM : HOUR_PM);
  }
}

void STM32RTC::syncTime(void)
{
  if(_configured) {
    hourAM_PM_t p = HOUR_AM;
    RTC_GetTime(&_hours, &_minutes, &_seconds, &_subSeconds, &p);
    _hoursPeriod = (p == HOUR_AM)? AM : PM;
  }
}

This code shows that _hoursPeriod is determined by syncTime() function first in setTime(). The _hoursPeriod is set in syncTime() and then setTime will assign its _hours, _minutes, and so forth if the user gave. Then setTime() calls RTC_SetTime() and this is a bit confusing. Apparently, in syncTime(), RTC_GetTime() was called before the RTC_SetTime(). Then _hoursPeriod will be a 'past AM/PM', not a 'present AM/PM'.

RTC_SetTime(_hours, _minutes, _seconds, _subSeconds, (_hoursPeriod == AM)? HOUR_AM : HOUR_PM);

I cannot understand what this code means, especially for _hoursPeriod part. If AM_PM is not assigned by user, then even if the input is 23:00:00, if the past time was 11:00:00, isn't _hoursPeriod "AM", not "PM"? On the other hand, if the past time was 16:00:00, _hoursPeriod will be "PM" and if the input is 11:00:00, the 'real' input is at "AM", not "PM". I hadn't reviewed the core's RTC source because anyway code works at the time, however this will cause some misunderstand.

[fpistm]

AM/PM is only used when you are in 12H format.

[charlie1024]

Then, in 24H functions(such as the above setHours()), the AM/PM is totally regardless of the RTC clock, right? I was wondering why 24H functions pass the AM/PM value even if they're for 24H format.

[fpistm]

This is the same HAL API for both format. https://github.com/stm32duino/Arduino_Core_STM32/blob/effb47cfb1afd79b5adcbd53cf4c6808b63e7a43/cores/arduino/stm32/rtc.c#L367

[charlie1024]

That makes sense. Actually I think some redundant functions could be integrated with some tricks. I also think this could be the developer's custom and I'm just a user of the library so I'll not regard about this seriously. However, if without the 12H format two functions such as STM32RTC::setHours(uint8_t hours) and STM32RTC::setHours(uint8_t hours, AM_PM period) do same thing, I just wonder why they're separated. I think this is due to the compatibility reason, is it right?

[fpistm]

Don't remember why. Probably to avoid members variables misalignments and RTC values.

[charlie1024]

Thank you for assistance. Anyway the library works without any issue.. Actually, what I want to suggest is not about the redundant functions. As I mentioned above, I think it could be better to give some freedom to set multiple values with only single call, instead of setTime() and setSubSeconds() call because two calls would need to unnecessary access to the register. Of course, whether this is accepted or not is just on your hand :)