-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RTEMS ntpd support #528
base: 7.0
Are you sure you want to change the base?
Conversation
✅ Build EPICS Base 7 base-7.0-1249 completed (commit a4db73945b by @kiwichris) |
Core Group comment, not a review/request: This PR contains an NTP client for use by RTEMS-5 targets. We might not need to support RTEMS-5 after RTEMS-6 has been released and integrated into Base. There seems to be an issue with the current osdTime code on RTEMS-posix that could be fixed to allow RTEMS-5 to use the EPICS-provided NTP time provider (less accurate than a proper NTP client), but that might be sufficient for any remaining RTEMS-5 users. |
e227857
to
08d3b2d
Compare
I have reviewed the warnings the static checker has raised and they seem like noise to me. |
❌ Build EPICS Base 7 base-7.0-1441 failed (commit 8f1811b17e by @kiwichris) |
Firstly, Is this PR relevant now that RTEMS 6.1 is released?
Is the added If it is new, and would be unique to epics-base, then I think it would be be a worthwhile exercise to follow up on these warnings. I agree that the texts of these Codacy warnings are unhelpful, but I think several are actionable in that what is being attempted could be accomplished in simpler ways. There is a lot of C string manipulation being added with this PR, with zero test coverage. This makes me nervous. |
✅ Build EPICS Base 7 base-7.0-1441 completed (commit 8f1811b17e by @kiwichris) |
New and I have reviewed each case again. My comments are below.
Case 1: The suggestion is the clear memory implies a needs to be clear after use. The memory is a local variable on a calling stack and goes out of scope when the called function returns. The call is Case 2: The buffer pointed to by Case 3: I will add a length to the call using Case 4: Looking at this again I think the Any suggestions on how to check the changes without pushing to this PR?
How would test coverage to this code be added to EPICS? Chris |
Looking more closely... do I understand that the |
For that matter, could the whole I don't like seeing configuration file contents hard-coded into epics-base as it prevents, or at least complicates site/instance configuration. eg. your |
epics-base has a framework for unit-testing, and many example in the tree. eg. There is so far no For code like this string parsing, which is not really RTEMS specific, I might structure this to build and run also as a host executable. (with the happy side of being much easier to develop and debug...) |
08d3b2d
to
532b0f6
Compare
Maybe the comment at the start of the file may help:
The config file fragment is present is to provide backward compatibility for existing users who want a simple NTP set up similar to the simple client Eric and other provided years ago. I suggest you provide external configuration files fit for you system including the leapyear file. If you want EPCIS to break backward compatibility just say but I did not think EPICS did that sort of thing so I went to the trouble of finding a solution. |
Yes. [ Also I have attempted to deal with the checker but it seems there is a limit to how far it can see. The buffer is being cleared and the
The NTP client is replacing what existed. If you do not want this monitoring and reporting of the NTP client state it can be removed or disabled by default. Users can be pointed to the the
There is an RTEMS shell command available in the net-services package however it is not monitoring the state. It should not be hard for someone who knows that area of EPICS to add it. You could also provide a suitable config file to provide external access and do the same thing with any |
I am not asking for removal. I am asking whether this (imo. expert debugging) information can simply be presented in a "raw" form with less effort? Your comment makes it look like the "raw" form is space separated key=value pairs. Potentially with more keys than would be parsed into * The variable list as returned by the `rl` ntpq command. The output is:
*
* associd=0 status=0615 leap_none, sync_ntp, 1 event, clock_sync, |
❌ Build EPICS Base 7 base-7.0-1450 failed (commit 3ccbe5c1e5 by @kiwichris) |
The commit 144f975 from the iocshSetError additions that the Core Group merged earlier today has caused a conflict in this branch, sorry @kiwichris, please rebase and force-push. Note that the Appveyor build failure was a single "Failed in 1 hr" false positive job; I was trying to re-run that job but it aborted with "Pull request #528 is non-mergeable." |
I am sorry, I misunderstood. This is a great discussion and well worth having.
This is correct. The ntpq documentation provides a complete list. On Linux, FreeBSD etc
Yes this would remove the need for that code however you also lose the ability to know the synchronized state and if the clock source is usable:
If you have more than one clock source there could be issues if this one cannot cleanly signal its state. This code currently does not propagate this state. I was waiting for this change and this discussion to explore what this means. I feel on RTEMS and in an embedded system this may be important. Currently the code makes the same assumption about the clock source Linux does. EPICS on Linux or Windows would use the OS system for time unless there is a specialized time source. RTEMS being an embedded RTOS does not come with external networked "clock management" as a standard feature. You need to embed it and that means you need to manage it. I suppose this thread of discussion ends up at the EPICS system level and what the core dev see as being needed? Does NTP clock management on RTEMS:
Items 1 and 2 means the parsing code can be removed and item 3 means it remains. Note, the monitoring being discussed uses the |
All good and thanks for the heads up. |
- This changes require libntp and that is available in the rtems-net-services package. The repository is avalable on gitlab.rtems.org and it builds for the legacy and libbsd networking stacks. - Configure ntpd with the following env variables: EPICS_TS_NTP_INET : NTP server EPICS_TS_NTP_CONF_FILE : ntpd configuration file EPICS_TS_NTP_LEAP_SECONDS_FILE : ntpd leap seconds file A configuration file is used if both a configuration file and server IP address are set.
532b0f6
to
f5319dc
Compare
I have rebased the PR against 7.0 of of today. The Codacy issues raised have me confused so I am not sure how to address them or how you report things like this to Codacy. Is this PR able to be merged? |
Thanks, we are getting confused by Codacy issues at the moment too. There's an error in the Codacy ![]() I cancelled the Appveyor build for this PR, the queue there is slow and it doesn't build the RTEMS code anyway. I don't think we'll let those weird Codacy issues stop us from merging this PR once we're happy with it. I haven't reviewed the latest changes here myself (just answering this for now) but I am putting RTEMS on our meeting Agenda for Wednesday. |
Then I am in favor of approaches 1 or 2 as I would prefer not to see this complex print-and-reparse logic in Base. Personally, I have no problem with allowing a read-only subset of NTP mode 6. imo. EPICS IOCs are by their nature always network attached, and thus always subject to possible DoS.
How about a simpler way to parse out this one key/value pair? Of course a regexp. Perhaps also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR able to be merged?
imo. no.
I'm sorry but I do not like the design being proposed. I would like to avoid our growing a custom, difficult to test, mostly hard coded, init system for RTEMS. This PR seems like a significant step in that direction.
#595 has my thinking about what an alternative design(s) could look like.
wrt. this PR specifically, I would want to see osdNTP_Monitor()
removed. I would also want to see some explanation of why osdNTP_Runner()
is needed. What all is the osdNTPPvt.config_source
state machine meant to handle?
In general, I would like to see a PR which starts with the absolute minimum needed to setup and call rtems_ntpd_run()
(assuming it can't already be called from the RTEMS shell). Then add only the simplest diagnostics necessary for a human to verify that the NTP client is running and synced. No new $EPICS_*
environment variables. No hardcoded /etc
file contents.
Thanks for the review and feedback.
Thanks but there is no need to be sorry. It is good we work towards a suitable design and solution. My response here is to try and present the reasons for the choices made.
From your ending comment I assume the hard coding reference is the configuration and leap second file fragments? The config files are in the NTP client configuration format. The file fragment is taken from the ntp01 test code in RTEMS. It is the way this stuff works on RTEMS. The code is only in the RTEMS POSIX directory. Does EPICS testing for RTEMS support networking protocols like NTP? If so I would love the details so I can take a look. Did the NTP client for the legacy stack Eric wrote have a set of tests? In the simplest configuration mode set the environment variable The single env var
I have responded in that issue and I agree with the approach. I am happy to consider changes however I would prefer I understand and we agree on what is acceptable first. The current approach attempts to:
How do you monitor the NTP client to determine the clock source is synchronized and the clock source is stable and usable? On an RTEMS system there is no ability to run some other process or use some other tool to monitor the state of NTP. I felt it important for systems engineers to know the state of the NTP client on RTEMS to aid integration as well as know the state. The monitoring and the EPICS command has proved to be valuable in the systems running this code in production. I added all I could because it was not easy to see what could be removed and yet useful. The data was put there by the NTP designers so I respected their decisions and provided that data to the EPICS command. If EPICS has suitable hooks I can use then yes lets move to using them. NTP running on RTEMS is a black box. I did look into inspecting the internal state of the client before adding the monitoring via a loopback socket however the NTP client code is not thread safe so there would have been race conditions.
The RTEMS now supports a complete NTP client and like an instance running on Unix it is configured with a couple of files, the The monitoring and triggering of configuration states lets a user add an environment variable independent of the state of the NTP client and the client will start. There is no In respect to #595 I did mention the timing of the network interface initialization and you could view the solution here as a self-contained instance of the same problem that code would face. If #595 handled initialization events and some sort of dispatch for RTEMS specific services the running code here could be removed.
I am not sure what that achieves as this PR provides that and then everything else that is needed? This code has been running in production systems no-stop from July 2024. As stated I added all the NTP fields to the command to avoid any extra overheads and complexity for EPICS users of this NTP client. They do not need a remote
Could you please indicate using the
How would the NTP client work without the configuration files it needs? I think a "hardcoded" restriction that bans publicly documented file formats will make RTEMS on EPICS difficult. Maybe the EPICS core devs should discuss what this means and then maybe reach out to the RTEMS community? This PR and #595 is aimed at cleaning up years of EPICS peeking under the hood at RTEMS and then breaking when RTEMS changes. Part of the solution where appropriate is to move to file formats the underlying software provides. Nothing here or in #595 has been created by RTEMS and in the case of There are parts of this code that could be moved to RTEMS net-services and test added if this helps EPICS and EPICS integration such as the default configuration for a single IP address and the monitoring? The monitoring thread would be best handled in EPICS given it can manage priorities. I think the ability to allow a user to provide a configuration file is a good addition for EPICS and should be kept. |
Good morning Chris,
I have not yet investigated where this suddenly comes from. I haven't seen it before. |
Thanks for the report. It looks resources or network related. NTP has kindly reporting its state to the console. I do not think it is related to this PR. I suggest moving this to the RTEMS Discourse site at https://users.rtems.org under Applications / EPICS category? |
@kiwichris You write quite an essay ;) I will try to pick a couple of essential points to address. Please feel free to call me out if I skip over something which you fell is essential. I'll start with the notion of configuration files.
I am referring these these configuration files. While this may be the way things are normally done with RTEMS in general, and I'm sure there are good reasons for this. eg. your general case is likely very general. However, for the much less general case of EPICS IOCs, I think there is a better way.
My objection is not to the content, but rather to the packaging. imo. these stop being practical configuration files when the content is embedded in C code! An analogy which might help you to understand what I see. This PR seems to me like a Linux distributor providing a single pre-built initramfs image, expecting each installation to unpack that image, manually edit some files, then pack it back up again. What I have in mind is analogous to the automation which avoids requiring end users to even know about initramfs. Now I don't know if we will be able to get all the way to that point, but I think it is necessary to try. I did not attempt to expand on these ideas in detail above because it will require more knowledge than I think is reasonable to ask of you ( @kiwichris ) about the EPICS build system, how the wider EPICS ecosystem is organized, and what our user's expectations are. Touching on a couple of points:
To give you an idea of where I think this design starts, I have encouraged @anjohnson (our Make maker) to think about how target filesystem fragments (including config files) should be placed into the installed tree of each EPICS module. |
In the EPICS world, this change action should be handled with an IOC shell command. Remember that every IOC instance will have an IOC shell script. This is how an instance is "personalized". So it is normal for an engineer setting up, or changing, an IOC to do so by issuing IOC shell commands. So while RTEMS shell is mostly used (I guess) for diagnostics, the IOC shell is first and foremost the central method for configuring an IOC instance. The many diagnostic tools are icing on that cake. Understand this, and all that it implies, is one of the big mental hurdles everyone has to clear in understanding the way EPICS IOCs work in practice. (so you are in good company if this seems foreign to you now)
Clearly. Otherwise you would not be trying to add a lite version to epics-base ;)
There isn't a way. The situation so far is imo. a mess of organic growth over many years. I am looking to exploit the disruption which is already being caused by the RTEMS 4 -> 6 transition to sweep some of this mess away, or at least isolate it along with the "legacy" network stack code.
Right, exactly my concern. There will never be one answer to a question like this. So my "answer" is to make it scriptable, and allow this scripting to be easily overridden/extended at various levels. |
Yes it was a lot but I felt it easier to lay out things in the hope it saves time.
I welcome your feedback because it makes me rethink how RTEMS integrates with users. The work in the PR is what is needed and that was goal. Your feedback highlights real issues and that is a good thing and it shows the work present needs to be broken up. I am happy to take this PR as a first pass base to work from.
Makes sense, thanks.
We could package them into EPCIS as you say but we need to preprocess the file to add the user's IP address from the boot environment when coping it to Another possibility is moving that code into RTEMS's net-services where the NTP code lives and handled there via an agreed interface work? This offers the benefit that each RTEMS release's config file format has to work and EPICS does not care.
The analogy works. If the file is in the EPICS configuration data do you see a user updating a
Fair. I am learning and welcome any guidance or insights so please call out any issues you see.
Agreed.
OK
Agreed and nice.
Sure and thanks. |
Thanks and that makes sense. There is a complication here. The RTEMS net-services wrapper around the NTP code provides the ability to stop and restart the service. How well it works depends on the options provided to the client. The NTP code is old and its clean up code when closing down is questionable. It appears to rely on a process exiting to clean up. We have looked into dealing with this and in some cases it works but you enter a deep hole with no bottom when attempting changes. As a result should we protect EPICS users and not allow runtime configuration changes? A change requires a reboot? Doing so removes the code.
You see my embedded background conflicting here. This is a nice feature.
The RTEMS shell is not a good example of a shell and yes it can run commands which is basically a hard coded text name to function caller. Having access via the EPICS
I am seeing it and starting to understand but I do not think I am there. I only use EPICS enough to deal with RTEMS related issues and it shows.
Haha yeah lets not.
Agreed
Excellent. I am onboard with this.
We saw the issue from the RTEMS perspective and added the net-services package as a place to smooth over the difference in the legacy, libbsd and even lwip stacks. I am happy to provide interfaces that can be used in a consistent way in EPICS. For example a call that probes an interface to see if it has an IP address. This is off topic for this PR and removal of the configuration probing code will be actioned. One item remaining. I can move the monitoring code to RTEMS net-services and then EPCIS can selectively decide on monitoring NTP for synchronized time. This would let EPICS control the synchronized state of the clock source. Does this work for you? |
Oh yes. Several ways at build and runtime. Most relevantly, the IOC shell expands environment variables as each line is executed. This is why the notion of eg. pre-populating the environment from eeprom is a natural one. Quite a lot can then be done with these values. Here is one intimidatingly complex example of a reusable IOC shell fragment. |
With RTEMS 6 this changes require
libntp
and that is available in thertems-net-services
package. The repository is available on https://gitlab.rtems.org and it builds for the legacy andlibbsd
networking stacks.RTEMS 5 builds the default POSIX support as RTEMS 5 does not have the kernel NTP support.
Configure ntpd with the following env variables:
EPICS_TS_NTP_INET
: NTP serverEPICS_TS_NTP_CONF_FILE
: ntpd configuration fileEPICS_TS_NTP_LEAP_SECONDS_FILE
: ntpd leap seconds fileA configuration file is used if both a configuration file and server IP address are set.
This PR depends on #375. I do not know how to make a merge train on GitHub.