AW: AW: [PATCH 12/12] spec/build/cpukit: Add option for enabling PPS synchronization

[Gabriel.Moyano at dlr.de]

> On 07/04/2022 11:57, Gabriel.Moyano at dlr.de wrote:
> >> On 07/04/2022 10:36, Gabriel Moyano wrote:
> >>> ---
> >>>    spec/build/cpukit/cpuopts.yml    |  2 ++
> >>>    spec/build/cpukit/optppssync.yml | 16 ++++++++++++++++
> >>>    2 files changed, 18 insertions(+)
> >>>    create mode 100644 spec/build/cpukit/optppssync.yml
> >>>
> >>> diff --git a/spec/build/cpukit/cpuopts.yml
> >>> b/spec/build/cpukit/cpuopts.yml index 301d49ccea..db0e05395f 100644
> >>> --- a/spec/build/cpukit/cpuopts.yml
> >>> +++ b/spec/build/cpukit/cpuopts.yml
> >>> @@ -49,6 +49,8 @@ links:
> >>>      uid: optparavirt
> >>>    - role: build-dependency
> >>>      uid: optposix
> >>> +- role: build-dependency
> >>> +  uid: optppssync
> >>>    - role: build-dependency
> >>>      uid: optprofiling
> >>>    - role: build-dependency
> >>> diff --git a/spec/build/cpukit/optppssync.yml
> >>> b/spec/build/cpukit/optppssync.yml
> >>> new file mode 100644
> >>> index 0000000000..6766cf0d40
> >>> --- /dev/null
> >>> +++ b/spec/build/cpukit/optppssync.yml
> >>> @@ -0,0 +1,16 @@
> >>> +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> >>> +actions:
> >>> +- get-boolean: null
> >>> +- env-enable: null
> >>> +- define-condition: null
> >>> +build-type: option
> >>> +copyrights:
> >>> +- Copyright (C) 2022 German Aerospace Center (DLR)
> >>> +default: false
> >>> +default-by-variant: []
> >>> +description: |
> >>> +  Enable time synchronization using a Pulse Per Second signal
> >>> +enabled-by: true
> >>> +links: []
> >>> +name: PPS_SYNC
> >>> +type: build
> >>
> >> If this should really be a CPU option, then it needs a proper RTEMS_* prefix.
> >
> > Do you mean that the name should be RTEMS_PPS_SYNC?
> 
> We should avoid new CPU options in general.

May I ask why?

> >
> >> Why do we need the option. What is the overhead if it is always enabled.
> >
> > The PPS_SYNC option comes from FREEBSD. Without it, you can still use the API. For example a task can wait for an event calling
> time_pps_fetch().
> > But if it is not enabled, hardpps() isn't called and time_freq is not updated.
> 
> Is something from PPS_SYNC used in the normal timekeeping?

Defining it just enables the time synchronization with a PPS signal. It can be defined by default or it can be added as an option to keep similarities with FreeBSD.
 
> >
> >> In general, the patch set lacks test cases.
> >
> > I was thinking that the next step could be to add a generic device which is required to use the API (a file descriptor is needed). This is
> something that wanted to ask in the mailing list first. When this device is added, also the test cases can be. Furthermore I wanted to
> split the submitted changes in order to make a review easier.
> 
> A file descriptor should be optional. It should be possible to add a PPS driver just using struct pps_state.

The struct pps_state type is used only internally. In order to be able to use the API a file descriptor is required (API functions start with time_pps_*()).
The "generic device" I have in mind will have an object of struct pps_state.