[PATCH 6/7] score: Always validate ticks in _TOD_Validate()

[Chris Johns]

On 7/9/21 3:34 pm, Sebastian Huber wrote:
> On 07/09/2021 05:11, Chris Johns wrote:
>> On 6/9/21 6:11 pm, Sebastian Huber wrote:
>>> On 06/09/2021 09:55, Chris Johns wrote:
>>>> On 6/9/21 3:49 pm, Sebastian Huber wrote:
>>>>> On 04/09/2021 06:20, Joel Sherrill wrote:
>>>>>>        > -    sc = _TOD_Validate(&temp_tod, TOD_ENABLE_TICKS_VALIDATION);
>>>>>>        > +    sc = _TOD_Validate(&temp_tod);
>>>>>>
>>>>>>       This has leaked out of the internal implementation interface. Should
>>>>>> it?
>>>>>>
>>>>>>       I prefer this does not happen and BSPs use an API.
>>>>>>
>>>>>>
>>>>>> I noticed this also. It needs an API.
>>>>> There is no RTEMS API to support writing realtime clock drivers currently.
>>>> Where did the RTC API connection comes from? I was only thinking of a call in
>>>> `rtems/clock.h` to validate time. I would not expect an RTC API to result in
>>>> the
>>>> validate call being moved from where it is.
>>>
>>> This is not the only _TOD_* stuff used by RTC drivers.
>>
>> A check shows there are 2 calls used and the bfin bsp can be ignored. Maybe I am
>> missing something here. Am I?
> 
> Yes, you missed the other uses of _TOD_* stuff:
> 
> bsps/arm/lpc32xx/rtc/rtc-config.c:  lpc32xx_rtc_set(_TOD_To_seconds(tod));
> bsps/shared/dev/rtc/ds1375.c:  secs = _TOD_To_seconds( time );
> bsps/shared/dev/rtc/ds1375.c:  secs = _TOD_To_seconds( &rtod );
> bsps/shared/dev/rtc/rtc-support.c:  rtems_time = _TOD_To_seconds( &rtems_tod );
> bsps/shared/dev/rtc/rtc-support.c:  rtc_time = _TOD_To_seconds( &rtc_tod );
> bsps/bfin/shared/dev/rtc.c: _TOD_Days_to_date[1][tod_temp.month] + tod_temp.day
> - 1;
> bsps/bfin/shared/dev/rtc.c:    if (days <= _TOD_Days_to_date[Leap_year][n+1]) {
> bsps/bfin/shared/dev/rtc.c:      tod_temp.day = days -
> _TOD_Days_to_date[Leap_year][n];
> bsps/bfin/shared/dev/rtc.c:  rtems_time = _TOD_To_seconds( &rtems_tod );
> bsps/bfin/shared/dev/rtc.c:  rtc_time = _TOD_To_seconds( &rtc_tod );
> 

This is what I saw and it not much that needs to be added to the header file.

>>
>>>>> I am not that sure if this is really a high priority issue.
>>>> If you mean an RTC API, sure I agree with that. If this is about dipping
>>>> directly into the score, I do not agree, it is always a priority.
>>>
>>> The _TOD_* stuff is probably used for decades in RTC drivers. Adding RTEMS API
>>> functions is a bit of work and all core developers are busy. I think it is
>>> unrealistic that someone will have time to work on this topic.
>>
>> I think our goals are not aligned. I am only asking the call be protected by an
>> API signature placed in rtems/clock.h. Is there a problem with doing this?
>>
>> /**
>>   * @brief Validates the time of day.
>>   *
>>   * @param tod is the reference to the time of day structure to validate or
>>   *   NULL.
>>   *
>>   * @retval RTEMS_SUCCESSFUL @a tod references a valid time of day.
>>   * @retval RTEMS_INVALID_CLOCK @a tod references an invalid time of day.
>>   * @retval RTEMS_INVALID_ADDRESS @a tod reference is @c NULL.
>>   */
>> rtems_status rtems_time_of_day_validate(rtems_time_of_day* tod);
>>
>> And the implementation is:
>>
>> rtems_status rtems_time_of_day_validate(
>>    rtems_time_of_day* tod
>> )
>> {
>>    return _TOD_Validate(tod);
>> }
> 
> The RTEMS API should be documented in the Classic API Guide. There should be
> also separate tests for API functions. API functions should be stable and cover
> important use cases of applications. I am really not sure if a function used
> only by two device drivers should be moved to the API.
> 

static void test_leap_year(void)
{
    rtems_status_code test_status;
    const rtems_time_of_day *problem = &problem_2100;
    const rtems_time_of_day *problem2 = &problem_2100_2;
    // 2100 is not a leap year, so it should have 28 days
    test_status = rtems_time_of_day_validate(problem);
    rtems_test_assert(test_status == RTEMS_SUCCESSFUL);
    test_status = rtems_time_of_day_validate(problem2);
    rtems_test_assert(test_status == RTEMS_INVALID_CLOCK);
}

?

Chris