Reactivate connection on powersave change, misc cleanup. #45
Conversation
| @@ -29,6 +29,10 @@ const ( | |||
| ConnCheckFilepath = "/etc/NetworkManager/conf.d/80-viam.conf" | |||
| ConnCheckContents = "[connectivity]\nuri=http://packages.viam.com/check_network_status.txt\ninterval=300\n" | |||
|
|
|||
| wifiPowerSaveFilepath = "/etc/NetworkManager/conf.d/81-viam-wifi-powersave.conf" | |||
There was a problem hiding this comment.
These belonged here with similar definitions for the other files.
Also, changed filename to avoid potential collisions with user-managed/written files.
| @@ -29,6 +29,10 @@ const ( | |||
| ConnCheckFilepath = "/etc/NetworkManager/conf.d/80-viam.conf" | |||
| ConnCheckContents = "[connectivity]\nuri=http://packages.viam.com/check_network_status.txt\ninterval=300\n" | |||
|
|
|||
| wifiPowerSaveFilepath = "/etc/NetworkManager/conf.d/81-viam-wifi-powersave.conf" | |||
| wifiPowerSaveContentsDefault = "# This file intentionally left blank.\n" | |||
There was a problem hiding this comment.
If not set, we basically want to not set anything. Explicitly setting 0 or 1 might still override another file in conf.d that a user put in place.
| // When true, it will disable power save for all wifi connections managed by NetworkManager. | ||
| DisableWifiPowerSave *bool `json:"disable_wifi_power_save"` | ||
| // If set, will explicitly enable or disable power save for all wifi connections managed by NetworkManager. | ||
| WifiPowerSave *bool `json:"wifi_power_save"` |
There was a problem hiding this comment.
Double negatives are... weird. Changed to a postive. True == on == powersave. Let me know if that doesn't sound okay though.
| @@ -16,7 +16,7 @@ func (w *Provisioning) startGRPC() error { | |||
| bind := PortalBindAddr + ":4772" | |||
| lis, err := net.Listen("tcp", bind) | |||
| if err != nil { | |||
| return errw.Wrapf(err, "error listening on: %s", bind) | |||
| return errw.Wrapf(err, "listening on: %s", bind) | |||
There was a problem hiding this comment.
A bunch of these scattered around. Errors no need to say the word "error" as it's implicit in the fact that it IS an error. Saw one in the new code, and cleaned up all my own uses of this elsewhere to match.
| } | ||
|
|
||
| // start portal with ssid list and known connections | ||
| if err := w.startPortal(inputChan); err != nil { | ||
| err = errors.Join(err, w.deactivateConnection(w.Config().HotspotInterface, w.Config().hotspotSSID)) | ||
| return errw.Wrap(err, "could not start web/grpc portal") | ||
| return errw.Wrap(err, "starting web/grpc portal") |
There was a problem hiding this comment.
Another type of minor cleanup/standardization... Wrapped errors should provide context, and nothing more, as the error itself says what went wrong.
| } | ||
|
|
||
| ssid := w.netState.ActiveSSID(w.cfg.HotspotInterface) | ||
| if w.connState.getConnected() && ssid != "" { |
There was a problem hiding this comment.
i guess we're checking with only do this if we're not in hotspot mode which is a good catch
Doing a PR to Ale's branch for discussion. Needs to merge before that (#43) merges.
Tested on my Pi 5 and it works as expected. Only "catch" now is that if powersave is set explicitly, then the setting is removed entirely, the last remaining state "remains" until the next reboot (as the driver itself initially turned on powersave during boot.) That is, assuming the user hasn't manually configured their own setting.