Review: Approve
This change looks reasonable to me, and the appveyor builds succeed. Given that this corrects a runtime bug, I think this is good enough.
The output of epicsTimeTest from one of the jobs includes:
> # Resolution 101 ns
> # epicsThreadSleep(0.0) Delta 500 ns
> # Small Delta 0 ns
I'm mildly surprised to see the Small Delta exactly zero given that this was run on a VM. For reference, my (Linux) laptop shows
> # Resolution 1 ns
> # epicsThreadSleep(0.0) Delta 76868 ns
> # Small Delta 79 ns
I wonder about the mixing of 64-bit integer and 64-bit floating point math in epicsMonotonicGet(). (see diff comment)
Diff comments:
> diff --git a/modules/libcom/src/osi/os/WIN32/osdMonotonic.c b/modules/libcom/src/osi/os/WIN32/osdMonotonic.c
> index cb84643..208c1aa 100644
> --- a/modules/libcom/src/osi/os/WIN32/osdMonotonic.c
> +++ b/modules/libcom/src/osi/os/WIN32/osdMonotonic.c
> @@ -39,15 +42,10 @@ epicsUInt64 epicsMonotonicResolution(void)
> epicsUInt64 epicsMonotonicGet(void)
> {
> LARGE_INTEGER val;
> - if(osdUsePrefCounter) {
> - if(!QueryPerformanceCounter(&val)) {
> - errMessage(errlogMinor, "Warning: failed to fetch performance counter\n");
> - return 0;
> - } else
> - return val.QuadPart;
> + if(!QueryPerformanceCounter(&val)) {
> + cantProceed("epicsMonotonicGet: Failed to read Windows Performance Counter\n");
> + return 0;
> } else {
> - epicsUInt64 ret = GetTickCount();
> - ret *= 1000000;
> - return ret;
> + return (epicsUInt64)(val.QuadPart * perfCounterScale + 0.5); /* return value in nanoseconds */
Since the integer value involved 64-bit, it seems like precision will eventually be lost while val.QuadPart gets high enough. I wonder how this would manifest? Effectively coarser resolution with longer uptime?
> }
> }
--
https://code.launchpad.net/~freddie-akeroyd/epics-base/+git/epics-base/+merge/391018
Your team EPICS Core Developers is subscribed to branch epics-base:7.0.
- References:
- [Merge] ~freddie-akeroyd/epics-base:fix_win32_monotonic_time into epics-base:7.0 Freddie Akeroyd via Core-talk
- Navigate by Date:
- Prev:
Build failed: epics-base base-fix_win32_monotonic_time-589 AppVeyor via Core-talk
- Next:
Re: [Merge] ~freddie-akeroyd/epics-base:fix_win32_monotonic_time into epics-base:7.0 Freddie Akeroyd via Core-talk
- Index:
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
2014
2015
2016
2017
2018
2019
<2020>
2021
2022
2023
2024
- Navigate by Thread:
- Prev:
Re: [Merge] ~freddie-akeroyd/epics-base:fix_win32_monotonic_time into epics-base:7.0 mdavidsaver via Core-talk
- Next:
Re: [Merge] ~freddie-akeroyd/epics-base:fix_win32_monotonic_time into epics-base:7.0 Freddie Akeroyd via Core-talk
- Index:
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
2014
2015
2016
2017
2018
2019
<2020>
2021
2022
2023
2024
|