Calnex: do not try to change settings for channels that aren't installed #477
Conversation
calnex/config/config.go
Outdated
| c.set(target, s, fmt.Sprintf("%s\\freq", ch.CalnexAPI()), "1 Hz") | ||
| c.set(target, s, fmt.Sprintf("%s\\suppress_steps", ch.CalnexAPI()), api.YES) | ||
| } | ||
| // log.Debugf("%s: look at changing channel %s", target,ch) |
There was a problem hiding this comment.
Could you please remove all commented out lines - it makes it harder to review
eb3080a to
682be9d
Compare
calnex/config/config.go
Outdated
| c.set(target, s, fmt.Sprintf("%s\\suppress_steps", ch.CalnexAPI()), api.YES) | ||
| } | ||
| // Check that physical channel (not virtual) is installed before trying to change any settings | ||
| if c.chGet(s, ch, "installed") == "1" { |
There was a problem hiding this comment.
Just a suggestion to make this changeset a lot smaller, how about
if c.chGet(s, ch, "installed") == "0" {
log.Debugf("%s: no change to channel %s it is not installed", target, ch)
continue
}
// <code as before>
Would this work?
There was a problem hiding this comment.
Well it could be != "1" as well
There was a problem hiding this comment.
Yes that helps with the changes being clearer. VS code hides some of the changes So looked more reasonable at my end
I have been able to remove some logic further down as well as it shouldn't get there as well when disabling unused channels the installed check is not needed.
calnex/config/config.go
Outdated
| // set the modified config | ||
|
|
There was a problem hiding this comment.
Was this an incorrect statement?
There was a problem hiding this comment.
I think I just caught an extra line removing some of my commented out debug code
682be9d to
1cb24c4
Compare
|
@PDeanCalnex looks much better, but I think a bunch of tests now broken. For example: |
|
@leoleovich has imported this pull request. If you are a Meta employee, you can view this in D84908650. |
|
Looks good overall, will wait for tests to complete and merge. Ok with you @PDeanCalnex? |
|
@leoleovich merged this pull request in 54b50e6. |
|
I'm doing a final check against a couple of units. Works fine against our latest code. A unit is just downgrading to R22.0.1 so I can do a final sanity check against that. |
|
I ran it against a random device on the fleet and it showed no changes, so should be good |
|
Then all good by me I think |
Summary
In our upcoming firmware release we have had to change how our settings files are applied and ignore any settings that are for channels that are not installed
This results in the GO tool constantly seeing some settings that it thinks are wrong for channels that are not installed so constantly stopping a measurement and applying a new settings file where some of the settings are being ignored.
These changes check for the installed setting for each channel being 1 before allowing changes to the expected settings for comparison before upload
There is a simpler way to mitigate this which is to remove the references to channels A and B so those channels are never set as they are the main culprits, but it is less robust, and would be less useful for anyone else using the applicaiton.
Test Plan
I have added to the unit tests. I can do some more testing if you are happy with this as the solution and not just removing channel A/B references.