Removed clock dependence on timestamper - necessary for multiport #598
Conversation
christopher-s-hall
commented
May 19, 2017
•
christopher-s-hall
commented
operation
Removed PHY compensation from timestamper handled in port and message
processing
Reduces copies of the PHY compensation data
Uses map object instead of various arrays and structs
Added link speed determination to Linux specific code used to correctly
compensate for PHY delay
Change sendPort from void to bool return value
Returns result of TX timestamp read
Return value still ignored (FUTURE work)
Common TX timestamp read function eliminates many "cut and paste"
instances
Added neighbor delay threshold and sync receipt threshold to port init
class avoiding chicken-egg port construction problem
Moved ethernet specific timestamp functions to EtherTimestamper class
Added simple link speed detection to Windows that *does not*
dynamically adjust
daemons/gptp/gptp_cfg.ini
Outdated
| @@ -31,19 +31,11 @@ syncReceiptThresh = 8 | |||
|
|
|||
| [eth] | |||
|
|
|||
| # PHY delay GB TX in nanoseconds | |||
| # PHY delay GB TX/RX in nanoseconds | |||
There was a problem hiding this comment.
Why were TX and RX combined? This change will break solutions (such as one we currently have) that use files to adjust the PHY delays, so I'd prefer not to change this without a compelling reason.
There was a problem hiding this comment.
I can back this out for now, but I don't think it's scalable to have fixed configuration options like this. For wireless, it'll be awkward. Eventually I think we should have something like phy_delay 1000000 384 184. What I proposed doesn't do this but it's part way.
I suppose we could have both?
There was a problem hiding this comment.
@bdthomsen, I saw that @christopher-s-hall did update the PHY delay information in the .ini files embedded in OpenAnvu. Are you saying you have various other .ini files scattered around, so having to sync all those to a new format would be painful?
There was a problem hiding this comment.
I can change the .ini file we are using. I was more concerned that others may be caught unaware by this change, as it does break backwards-compatibility as written.
There was a problem hiding this comment.
I've pushed changes that keep the old format and add a new format:
phy_delay = LINKSPEED_1G 184 382
The there are some predefined speeds that can be used like above or the speed can be entered numerically.
There was a problem hiding this comment.
Thanks @christopher-s-hall. I missed this last week. I think we are good to merge as is then.
|
My only comment is more of an observation. It is that C++ maps are now used in the gPTP build. I don't think that should be a problem, but in the future a "lighter" implementation may be required if someone wants to embedded gPTP in small device with limited library space. Not a show stopper in my opinion. |
christopher-s-hall
force-pushed
the
open-avb-next
branch
6 times, most recently
from
414dab3 to
795eaec
Compare
|
@andrew-elder Does using the STL map really increase code size or memory usage that much compared to writing a map class? I don't know the answer. Could C++ be used to build for such a device? I think we may already be using STL lists and we are definitely using STL strings. |
|
We've apparently had STL map in avbts_osnet.hpp for a very long time. I'm not sure if our gPTP implementation would be suitable for such a highly constrained environment, but I don't think this makes it any less so. C++ can create builds as small as, or sometimes smaller than, C builds. Depends on the compiler implementation and the libraries used. But many real-time embedded systems will avoid STL or use an embedded-specific implementation of a subset of it. Sometimes it's to avoid dynamic allocation, sometimes to avoid exceptions, etc. I don't know all the details, but I do know that C++ use in small embedded systems in rising, and embedded C++ compilers are getting very good at emitting tight code. |
|
It appears to me that this branch has the same issue that's addressed in #600. If we merge that one, this will undo it. If you have an alternate fix for it (e.g. add const to the Linux HWTimestamper_adjclockrate member function so it overrides the virtual method on the parent) you could add it in. Or maybe I missed something and this isn't vulnerable to the issue, but it looks to me like it could be. |
|
#600 isn't merged yet. This is based on the current open-avb-next head, but I can add a fix to this. |
|
My vote would be that we revert the .ini file format change and then go ahead and merge this pull request. The issue addressed in #600 can be reviewed and merged separately. If we do it after this pull request we won't have any issues with accidentally backing out bugfixes. |
|
I see @christopher-s-hall has already pushed changes to the .ini format that I think are ok. I need one more sign-off and we can merge. Thanks for the STL discussion. As I said, I don't think it is an issue at all, but I wanted to bring it up in case someone else had a stronger opinion on the subject. |
