Integrate T-BC on Perla2 platform #119
Conversation
|
Thanks for your PR, |
vitus133
force-pushed
the
perla-integ
branch
from
a7a7065 to
58b16f9
Compare
greyerof
left a comment
greyerof
left a comment
There was a problem hiding this comment.
Some more nits plus some (strong) recommendations to avoid hidding errors.
pkg/daemon/daemon.go
Outdated
| // For T-BC mode with hardwareconfig, derive interfaces from hardwareconfig structure | ||
| // instead of analyzing ts2phc configuration file | ||
| var interfacesToUse config.IFaces | ||
| if profileClockType == TBC && dn.hardwareConfigManager != nil && |
There was a problem hiding this comment.
I don't think you need this check dn.hardwareConfigManager != nil. You're already using it without any check at line 668 right at the very beginning of this func.
Also, I've seen this check in other functions, but I don't think it's necessary. The hardwareconfig.NewHardwareConfigManager() that sets this field in the daemon's field would have probably panic'ed right at the creation of the daemon.
There was a problem hiding this comment.
Right, dn.hardwareConfigManager is never nil, as New() always allocates the manager regardless of whether any HardwareConfig objects exist. Removing it everywhere
pkg/daemon/daemon.go
Outdated
| inSyncConditionTh, inSyncConditionTimes) | ||
| glog.Infof("depending on %s", dpllDaemon.DependsOn()) | ||
| // Set hardwareconfig handler if hardwareconfig manager is available | ||
| if dn.hardwareConfigManager != nil { |
pkg/hardwareconfig/utils.go
Outdated
|
|
||
| // Step 2: PERLA workaround - Check if this is an E825 device | ||
| // For E825 devices, there's no direct NIC-DPLL association, so we look for the zl3073x DPLL | ||
| lspciOutput, lspciErr := commandExecutor.Execute("lspci", "-s", busAddr) |
There was a problem hiding this comment.
You're not handling here a possible error value in lspciErr. Why not just checking it first? Also (nit), you can also reuse the "err" variable name.
lspciOutput, err := commandExecutor.Execute("lspci", "-s", busAddr)
if err != nil {
return 0, fmt.Errorf("failed to run lspci -s %s, output: %s, err: %w", busAddr, lspciOutput, err)
}
if strings.Contains(lspciOutput, "E825") {
...
pkg/hardwareconfig/utils.go
Outdated
| cache := pinCache | ||
| if cache == nil { | ||
| var cacheErr error | ||
| cache, cacheErr = GetDpllPins() | ||
| if cacheErr != nil { | ||
| glog.Warningf("PERLA workaround: failed to get pin cache: %v, falling back to serial number approach", cacheErr) | ||
| cache = nil | ||
| } | ||
| } | ||
|
|
||
| if cache != nil { | ||
| // Look for first pin with moduleName == "zl3073x" | ||
| for clockID, pins := range cache.Pins { | ||
| for _, pin := range pins { | ||
| if pin.ModuleName == "zl3073x" { | ||
| glog.Infof("PERLA workaround: Found zl3073x DPLL with clock ID %#x, associating with E825 interface %s", clockID, iface) | ||
| return clockID, nil | ||
| } | ||
| } | ||
| } | ||
| glog.Warningf("PERLA workaround: No zl3073x DPLL found in pin cache, falling back to serial number approach") | ||
| } |
There was a problem hiding this comment.
Nit: Hide all this logic in a separate func like getPERLAClockIDFromPinCache() or similar.
if strings.Contains(lspciOutput, "E825") {
glog.Infof("Detected E825 device on %s (interface %s), using PERLA workaround", busAddr, iface)
clockId, err := getPERLAClockIDFromPinCache(pinCache)
if err != nil {
glog.Warningf("PERLA workaround: failed to get clock ID from pin cache: %v, falling back to serial number approach", err)
} else {
return clockId, nil
}
}
pkg/plugin/plugin.go
Outdated
| pluginFunc := pluginObject.OnPTPConfigChange | ||
| if pluginFunc != nil { | ||
| pluginFunc(pm.Data[pluginName], nodeProfile) | ||
| _ = pluginFunc(pm.Data[pluginName], nodeProfile) |
There was a problem hiding this comment.
A warning log trace is worth here instead of just hidding the error in the "_" var. Same with the others (lines 57 and 67).
There was a problem hiding this comment.
I removed this change from the set - it was unintentional. Joseph already doing something in the area of plugin failures, I don't want to create merge problems for him
vitus133
force-pushed
the
perla-integ
branch
from
6c64ca6 to
5527ead
Compare
vitus133
force-pushed
the
perla-integ
branch
from
8e8de7c to
b95d1e1
Compare
greyerof
left a comment
greyerof
left a comment
There was a problem hiding this comment.
Looks good to me, but I'm not familiar enough with this new controller so I can only review basic go code. In that regards, I can see more code blocks that could be moved to separate functions to improve readability/testability.
vitus133
force-pushed
the
perla-integ
branch
from
b95d1e1 to
686866a
Compare
vitus133
force-pushed
the
perla-integ
branch
2 times, most recently
from
93ea077 to
5961889
Compare
Assisted by Cursor AI Add Perla2 vendor definitions Integrate on the target Signed-off-by: Vitaly Grinberg <[email protected]>
vitus133
force-pushed
the
perla-integ
branch
from
5961889 to
cc8ad4e
Compare
4 participants
Assisted by Cursor AI
Add Perla2 vendor definitions
Integrate T-BC on the target
/cc @lack @nocturnalastro @josephdrichard @jzding