![[RTEMS Logo]](/images/logo.png){kind=logo}
#2348 closed enhancement (fixed)
Timecounter: Add NTP support
| Reported by: | Sebastian Huber | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 6.1 |
| Component: | score | Version: | 4.11 |
| Severity: | normal | Keywords: | qualification |
| Cc: | Blocked By: | ||
| Blocking: |
Description
The FreeBSD timecounter implementation supports the NTP. This support is currently disabled in RTEMS.
Change History (23)
comment:1 Changed on 08/14/17 at 00:08:25 by Chris Johns
| Milestone: | 5.0 → Indefinite |
|---|---|
| Version: | 4.11 |
comment:2 Changed on 10/27/21 at 00:47:22 by Chris Johns
comment:3 follow-up: 4 Changed on 10/27/21 at 05:37:47 by Sebastian Huber
Good timing, I recently updated kern_tc.c to the latest FreeBSD version:
https://git.rtems.org/sebh/rtems.git/log/?h=timecounter-2021
I just didn't send the patch set for review yet, since nobody commented on the new clock manager directives so far:
https://lists.rtems.org/pipermail/devel/2021-October/069697.html
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
In this patch set I removed the NTP code block since nobody was interested in NTP support in the last 6 years:
This enabled a performance optimization here:
The performance optimization is important for targets with slow 64-bit divisions (which are most RTEMS targets). If you want to add NTP support, then we have to restructure the FreeBSD code a bit. This can be integrated in FreeBSD. I can do this it just depends on how urgent it is.
In a first approach the proposed callback makes sense. If someone using lwIP needs NTP support, then we can add kern_ntptime.c to RTEMS.
comment:4 follow-up: 6 Changed on 10/27/21 at 06:33:28 by Chris Johns
Replying to Sebastian Huber:
Good timing, I recently updated kern_tc.c to the latest FreeBSD version:
It does seem so. :)
https://git.rtems.org/sebh/rtems.git/log/?h=timecounter-2021
I just didn't send the patch set for review yet, since nobody commented on the new clock manager directives so far:
I only suspected something was in the works because of the posts to FreeBSD hackers. It would be good to resolve the PTP support.
https://lists.rtems.org/pipermail/devel/2021-October/069697.html
I saw this but I did not look closely. I did not know it was a request for a new feature. I will add it to my list so sorry about that.
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
Why the special exception here?
My focus for the leadership group, including you, is to set policies for this sort of thing and then having everyone accept and adhere to them. It is simpler and more democratic to do this. My concern is having a rule that creates an exceptions because it makes things complicated. I know this is a single file but there are other places where more code comes across and this is where things become confusing.
In this patch set I removed the NTP code block since nobody was interested in NTP support in the last 6 years:
It is needed. I have openly discuss this topic ...
https://lists.rtems.org/pipermail/devel/2021-March/065608.html
The PTP changes for RTEMS have only just been posted upstream ..
https://github.com/ptpd/ptpd/pull/85
Gabriel has done a fantastic job getting my hacks into a suitable state they would be pushed upstream.
This enabled a performance optimization here:
The performance optimization is important for targets with slow 64-bit divisions (which are most RTEMS targets). If you want to add NTP support, then we have to restructure the FreeBSD code a bit. This can be integrated in FreeBSD. I can do this it just depends on how urgent it is.
I have nothing to offer here and will happily be guided by you. I am sorry I have no time to spend looking at the detail.
In a first approach the proposed callback makes sense. If someone using lwIP needs NTP support, then we can add kern_ntptime.c to RTEMS.
The support being added is LibBSD so I am not sure where kern_ntptime.c is being added. Maybe Gabriel can be comment more on this.
comment:5 follow-up: 7 Changed on 10/27/21 at 12:03:51 by GabrielMoyano
kern_ntptime.c is ported from freebsd-org to freebsd in rtems-libbsd. The last commits of this branch https://github.com/GabrielDai/rtems-libbsd/tree/ptpd show the changes.
Regarding adding the function ntp_update_second() in score, do you know where the function pointer should be updated (to point to ntp_update_second() in kern_ntptime.c)?
comment:6 follow-up: 8 Changed on 10/28/21 at 05:25:49 by Sebastian Huber
Replying to Chris Johns:
Replying to Sebastian Huber:
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
Why the special exception here?
My focus for the leadership group, including you, is to set policies for this sort of thing and then having everyone accept and adhere to them. It is simpler and more democratic to do this. My concern is having a rule that creates an exceptions because it makes things complicated. I know this is a single file but there are other places where more code comes across and this is where things become confusing.
There is not just one rule involved here. The timekeeping is an important service of an operating system. It should be visible in the internal documentation (Doxygen). The file history shows that there is no issue with keeping it in synchronization with FreeBSD.
comment:7 Changed on 10/28/21 at 05:35:06 by Sebastian Huber
Replying to GabrielMoyano:
kern_ntptime.cis ported from freebsd-org to freebsd in rtems-libbsd. The last commits of this branch https://github.com/GabrielDai/rtems-libbsd/tree/ptpd show the changes.
Regarding adding the function
ntp_update_second()inscore, do you know where the function pointer should be updated (to point tontp_update_second()inkern_ntptime.c)?
The callback for ntp_update_second() is currently not implemented. I would like to integrate my patch set which updates kern_tc.c to the latest FreeBSD version first. I would like to also try to rearrange the code a bit in the FreeBSD upstream. For libbsd my approach would be to add an internal API to #include <rtems/score/timecounter.h> which can be used to set the NTP callback in kern_ntptime.c.
comment:8 follow-up: 9 Changed on 10/28/21 at 07:47:23 by Chris Johns
Replying to Sebastian Huber:
Replying to Chris Johns:
Replying to Sebastian Huber:
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
Why the special exception here?
My focus for the leadership group, including you, is to set policies for this sort of thing and then having everyone accept and adhere to them. It is simpler and more democratic to do this. My concern is having a rule that creates an exceptions because it makes things complicated. I know this is a single file but there are other places where more code comes across and this is where things become confusing.
There is not just one rule involved here. The timekeeping is an important service of an operating system. It should be visible in the internal documentation (Doxygen). The file history shows that there is no issue with keeping it in synchronization with FreeBSD.
This is not about the history. The history of this file reflects the quality of the person currently doing the work and it is a very high standard. Expecting that standard everywhere is asking a lot. I think we need to document how we integrate external code and for that I was considering the following as the basis:
https://git.rtems.org/rtems-libbsd/tree/CONTRIBUTING.md#n248
I would be interested to understand how this score file or files like it could be handled by a rule or rules? A rule in the ticket may not be clean and suitable documentation however it provides me with a means to show what we consider as OK if you could provide this.
comment:9 follow-up: 12 Changed on 10/28/21 at 09:17:20 by Sebastian Huber
Replying to Chris Johns:
Replying to Sebastian Huber:
Replying to Chris Johns:
Replying to Sebastian Huber:
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
Why the special exception here?
My focus for the leadership group, including you, is to set policies for this sort of thing and then having everyone accept and adhere to them. It is simpler and more democratic to do this. My concern is having a rule that creates an exceptions because it makes things complicated. I know this is a single file but there are other places where more code comes across and this is where things become confusing.
There is not just one rule involved here. The timekeeping is an important service of an operating system. It should be visible in the internal documentation (Doxygen). The file history shows that there is no issue with keeping it in synchronization with FreeBSD.
This is not about the history. The history of this file reflects the quality of the person currently doing the work and it is a very high standard. Expecting that standard everywhere is asking a lot. I think we need to document how we integrate external code and for that I was considering the following as the basis:
https://git.rtems.org/rtems-libbsd/tree/CONTRIBUTING.md#n248
I would be interested to understand how this
scorefile or files like it could be handled by a rule or rules? A rule in the ticket may not be clean and suitable documentation however it provides me with a means to show what we consider as OK if you could provide this.
The libbsd contributing rules make sense for libbsd which deals with a couple of hundred files and a semi-automatic synchronization support.
External files placed in the score should integrate into the score documentation and be maintainable. Here we need a bit more flexibility.
comment:10 follow-up: 11 Changed on 10/28/21 at 09:33:48 by Sebastian Huber
I updated the timecounter update branch to include support for an NTP update second handler:
https://git.rtems.org/sebh/rtems.git/log/?h=timecounter-2021
I wait for some feedback on the FreeBSD mailing list before I send the patch set for review.
comment:11 follow-up: 14 Changed on 10/28/21 at 11:29:14 by GabrielMoyano
Replying to Sebastian Huber:
I updated the timecounter update branch to include support for an NTP update second handler:
https://git.rtems.org/sebh/rtems.git/log/?h=timecounter-2021
This looks nice, thx.
Now, I guess that one option is to call _Timecounter_Set_NTP_update_second() just before starting ptpd
I wait for some feedback on the FreeBSD mailing list before I send the patch set for review.
ok
comment:12 Changed on 10/28/21 at 22:57:18 by Chris Johns
Replying to Sebastian Huber:
Replying to Chris Johns:
Replying to Sebastian Huber:
Replying to Chris Johns:
Replying to Sebastian Huber:
The kern_tc.c is quite close to the original FreeBSD code and the Doxygen comment block on top of the file is not an issue.
Why the special exception here?
My focus for the leadership group, including you, is to set policies for this sort of thing and then having everyone accept and adhere to them. It is simpler and more democratic to do this. My concern is having a rule that creates an exceptions because it makes things complicated. I know this is a single file but there are other places where more code comes across and this is where things become confusing.
There is not just one rule involved here. The timekeeping is an important service of an operating system. It should be visible in the internal documentation (Doxygen). The file history shows that there is no issue with keeping it in synchronization with FreeBSD.
This is not about the history. The history of this file reflects the quality of the person currently doing the work and it is a very high standard. Expecting that standard everywhere is asking a lot. I think we need to document how we integrate external code and for that I was considering the following as the basis:
https://git.rtems.org/rtems-libbsd/tree/CONTRIBUTING.md#n248
I would be interested to understand how this
scorefile or files like it could be handled by a rule or rules? A rule in the ticket may not be clean and suitable documentation however it provides me with a means to show what we consider as OK if you could provide this.
The libbsd contributing rules make sense for libbsd which deals with a couple of hundred files and a semi-automatic synchronization support.
External files placed in the score should integrate into the score documentation and be maintainable. Here we need a bit more flexibility.
This makes sense. Should the documentation should be limited to a single block at the start of the file and else where in the file the LibBSD rules apply?
comment:13 Changed on 11/15/21 at 08:16:56 by Sebastian Huber <sebastian.huber@…>
In ffb8833d/rtems:
comment:14 follow-up: 15 Changed on 11/15/21 at 08:19:27 by Sebastian Huber
Replying to GabrielMoyano:
Replying to Sebastian Huber:
I updated the timecounter update branch to include support for an NTP update second handler:
https://git.rtems.org/sebh/rtems.git/log/?h=timecounter-2021
This looks nice, thx.
Now, I guess that one option is to call _Timecounter_Set_NTP_update_second() just before starting ptpd
I wait for some feedback on the FreeBSD mailing list before I send the patch set for review.
ok
I checked in the patch set. The kern_tc.c is now fully synchronized with the latest FreeBSD version. You can now install an NTP update second handler via _Timecounter_Set_NTP_update_second() from #include <rtems/score/timecounter.h>.
comment:15 follow-up: 17 Changed on 11/15/21 at 10:56:37 by GabrielMoyano
I checked in the patch set. The kern_tc.c is now fully synchronized with the latest FreeBSD version. You can now install an NTP update second handler via
_Timecounter_Set_NTP_update_second()from#include <rtems/score/timecounter.h>.
Thank you very much Sebastian.
Are the changes portable to the branch 5? Our development is based on that branch and it will be very nice to have them there too
comment:16 Changed on 11/15/21 at 11:25:13 by Sebastian Huber
| Milestone: | Indefinite → 6.1 |
|---|---|
| Version: | → 4.11 |
comment:17 follow-up: 18 Changed on 11/15/21 at 11:27:10 by Sebastian Huber
Replying to GabrielMoyano:
I checked in the patch set. The kern_tc.c is now fully synchronized with the latest FreeBSD version. You can now install an NTP update second handler via
_Timecounter_Set_NTP_update_second()from#include <rtems/score/timecounter.h>.
Thank you very much Sebastian.
Are the changes portable to the branch 5? Our development is based on that branch and it will be very nice to have them there too
I think the patch set is back portable to RTEMS 5 without breaking the ABI/API. Just clone this ticket and set the mile stone to 5.2. Then try to back port all the changes or a subset and send it for review. I have no time to work on it.
comment:18 Changed on 11/15/21 at 11:40:42 by GabrielMoyano
I think the patch set is back portable to RTEMS 5 without breaking the ABI/API. Just clone this ticket and set the mile stone to 5.2. Then try to back port all the changes or a subset and send it for review. I have no time to work on it.
Thx. I'll do so. Here the ticket https://devel.rtems.org/ticket/4549#ticket
comment:21 Changed on 02/21/22 at 13:15:14 by Sebastian Huber <sebastian.huber@…>
In 8f1e8f8f/rtems:
comment:22 Changed on 06/07/22 at 06:54:14 by Sebastian Huber
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:23 Changed on 10/31/23 at 11:45:12 by Sebastian Huber
| Keywords: | qualification added |
|---|
There needs to be an interface in the
scoreto provide access tontp_update_second(). The call may not be present in the kernel sources, it will mostly likely be provided by an application or an external library such aslibbsd.a.I suggest a file is added to the
scorethat containsntp_update_second()and that function checks a function pointer and if set the function is called. This bringskern_tc.ccloser to upstream FreeBSD sources.While on the topic of
kern_tc.cand being as close to FB as we can, I think the RTEMS doxygen comments should be removed. We should maintain a common policy for imported sources like this.