-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APP-7635]: Define type for managing bluetooth WiFi provisioning. #62
base: main
Are you sure you want to change the base?
Conversation
…ed interface for OS abstraction).
…irware differences.
Note to reviewers: once we can agree upon the interface(s) added here, I will add unit testing to make it lock tight. |
f5f1be7
to
5d57942
Compare
5d57942
to
c5c1bc6
Compare
"fmt" | ||
) | ||
|
||
type bluetoothService interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is against Golang best practice to have unexported interfaces, and I'm far from an expert using generic types, so let me know if there are better ways to be designing this feature.
The purpose of this layer of abstraction was to protect the Linux-specific BT stuff from the core implementation of our BluetoothWiFiProvisioner
interface. It isn't exported because I don't expect anyone to need it. If you have more specific questions, let me know, and I'll dive in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tough for me to tell what's going on here without the implementer of this bluetoothService
interface, but I'm definitely suspicious of the need for any interface at all.
Will you have multiple implementers of bluetoothService
? Or multiple of BluetoothWiFiProvisioner
?
Using interfaces to have a "pimpl" (private implementation + public methods) design is definitely against Go best practices, but maybe I'm misunderstanding what you're doing here. Interfaces are generally helpful for accepting multiple types of things that implement a common set of methods, not for hiding implementations. Apologies if you know these things already, but figured I'd come out strong and say I'm suspicious of the patterns you're using here wrt to the two interfaces (and one that's unexported.)
I think my concern extends to your use of generics, as they seem to be how you're getting around your usage of interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for diving in --- and definitely appreciate explanations of pimpl and best practices because I'm always looking to level up in Golang. I've used pimpl interfaces in the past because I liked how it made testing modular, so that's what I was going for --- and happy to learn that's not how it should be done.
I added the bluetoothService
interface with the idea that some day we could support more than just Linux. The implementation I had in mind was linuxBluetoothService
, windowsBluetoothService
, etc., so that we could abstract away OS-specific bluetooth differences from the business logic inside of BluetoothWiFiProvisioner
. That being said, it is super unlikely we get to that point soon. So the only real reason I could have for adding it is to make unit tests not reliant on the OS, but even that seems like overkill.
Changes I will make:
- Remove the
BluetoothWiFiProvisioner
interface and make the corresponding private impl public - Remove the
bluetoothService
interface and instead add the methods I had in mind as private methods on theBluetoothWiFiProvisioner
type so they're only accessible here
} | ||
|
||
// BluetoothWiFiProvisioner provides an interface for managing the bluetooth (bluetooth-low-energy) service as it pertains to WiFi setup. | ||
type BluetoothWiFiProvisioner interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the exported interface which is to be consumed in the provisioning loop. In the interest of not "muddying" our provisioning loop flow, I keep this interface very simple. It only has things that you would need from the perspective of the provisioning loop, and it doesn't include anything about BT peripheral advertisement, characteristics, etc.
I get into the more "under-the-hood" functionality in the bluetooth_service.go
file. There, I also have an interface so I can abstract away all Linux OS stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing some of the picture here, but I thought I'd leave a round of comments before I'm OOO for most of next week. Feel free to ignore anything, too, more @Otterverse's + your code than mine.
"fmt" | ||
) | ||
|
||
type bluetoothService interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tough for me to tell what's going on here without the implementer of this bluetoothService
interface, but I'm definitely suspicious of the need for any interface at all.
Will you have multiple implementers of bluetoothService
? Or multiple of BluetoothWiFiProvisioner
?
Using interfaces to have a "pimpl" (private implementation + public methods) design is definitely against Go best practices, but maybe I'm misunderstanding what you're doing here. Interfaces are generally helpful for accepting multiple types of things that implement a common set of methods, not for hiding implementations. Apologies if you know these things already, but figured I'd come out strong and say I'm suspicious of the patterns you're using here wrt to the two interfaces (and one that's unexported.)
I think my concern extends to your use of generics, as they seem to be how you're getting around your usage of interfaces.
} | ||
|
||
// Update updates the list of networks that are advertised via bluetooth as available. | ||
func (bwp *bluetoothWiFiProvisioner[T]) RefreshAvailableNetworks(ctx context.Context, awns *AvailableWiFiNetworks) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method probably doesn't need a context if it's not passing it anywhere and is just immediately checking if it has errored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removed from there and passing it a layer below.
wg.Add(2) | ||
utils.ManagedGo( | ||
func() { | ||
ssid, ssidErr = waitForBLEValue(ctx, bwp.svc.readSsid, "ssid") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one of these four goroutines errors, do you want the other goroutines to stop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - yes. That's right because if it enters that line it means we're requiring that value to be read properly.
} | ||
|
||
// waitForBLE is used to check for the existence of a new value in a BLE characteristic. | ||
func waitForBLEValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] This function isn't really waiting for any BLE value, it's just calling a callback in a loop and continuing if there's a certain type of error. I wonder if the name should reflect that a bit better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - will make it more accurate and less bias to how its used.
"sync" | ||
"time" | ||
|
||
"github.com/pkg/errors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Not sure what agent's pattern is, but generally the standard library errors
is a little more idiomatic.
switch os := runtime.GOOS; os { | ||
case "linux": | ||
fallthrough | ||
case "windows": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
… is yet to be implemented.
2e5c12d
to
80d0929
Compare
…fix used elsewhere in the Agent.
…t sending information to a closed channel.
"errors" | ||
|
||
errw "github.com/pkg/errors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied this (including the errw
prefix) from elsewhere in the Agent. The "github.com/pkg/errors" package has errors.WithMessage(err, "some wrapping message")
and errors.Errorf("message with arg %s", arg)
(which do not exist in the standard "errors" package).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that using errw is just so we can migrate old code without too much hassle, but for new code you can use fmt.Errorf
// An easy way to create wrapped errors is to call [fmt.Errorf] and apply
// the %w verb to the error argument:
//
// wrapsErr := fmt.Errorf("... %w ...", ..., err, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Didn't know it was doable that way. Will change to this, thanks.
subsystems/provisioning/bluetooth/bluetooth_wifi_provisioner.go
Outdated
Show resolved
Hide resolved
…d and rename waitForBLEValue to retryCallbackOnEmptyCharacteristicError.
subsystems/provisioning/bluetooth/bluetooth_wifi_provisioner_utils.go
Outdated
Show resolved
Hide resolved
subsystems/provisioning/bluetooth/bluetooth_wifi_provisioner.go
Outdated
Show resolved
Hide resolved
|
||
// AvailableWiFiNetworks represent the networks that the device has detected (and which may be available for connection). | ||
type AvailableWiFiNetworks struct { | ||
Networks []*struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can re-use this type
agent/subsystems/provisioning/definitions.go
Line 112 in 90b7f59
type NetworkInfo struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above with regards to import cycles. Can use, but would need to pull that out. Otherwise, maybe we should just be moving this code into the existing provisioning
package as its own file where it can also access those values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into provisioning
and reused this type.
) | ||
|
||
// Credentials represent the minimum required information needed to provision a Viam Agent. | ||
type Credentials struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets reuse
agent/subsystems/provisioning/definitions.go
Line 257 in 90b7f59
type userInput struct { |
These types will be the same in the BLE and the Hotspot flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use that type but will have to move it out of the provisioning
package and into an isolated package where both can import it (to get around an import cycle). Does that sound like something you would want here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into provisioning
and reused this type.
if err := bwp.startAdvertisingBLE(ctx); err != nil { | ||
return err | ||
} | ||
bwp.enableAutoAcceptPairRequest() // Async goroutine (hence no error check) which auto-accepts pair requests on this device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean we won't provide helpful logging to the users? or that part will be implemented in enableAutoAcceptPairRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging is included in the implementation of enableAutoAcceptPairRequest
. The stacked PR containing its impl is here: #69.
wg.Wait() | ||
return &Credentials{ | ||
Ssid: ssid, Psk: psk, RobotPartKeyID: robotPartKeyID, RobotPartKey: robotPartKey, | ||
}, multierr.Combine(ssidErr, pskErr, robotPartKeyIDErr, robotPartKeyErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use errors.Join
if requiresWiFiCredentials { | ||
wg.Add(2) | ||
utils.ManagedGo( | ||
func() { | ||
if ssid, ssidErr = retryCallbackOnExpectedError( | ||
ctx, bwp.readSsid, &emptyBluetoothCharacteristicError{}, "failed to read ssid", | ||
); ssidErr != nil { | ||
cancel() | ||
} | ||
}, | ||
wg.Done, | ||
) | ||
utils.ManagedGo( | ||
func() { | ||
if psk, pskErr = retryCallbackOnExpectedError( | ||
ctx, bwp.readPsk, &emptyBluetoothCharacteristicError{}, "failed to read psk", | ||
); pskErr != nil { | ||
cancel() | ||
} | ||
|
||
}, | ||
wg.Done, | ||
) | ||
} | ||
if requiresCloudCredentials { | ||
wg.Add(2) | ||
utils.ManagedGo( | ||
func() { | ||
if robotPartKeyID, robotPartKeyIDErr = retryCallbackOnExpectedError( | ||
ctx, bwp.readRobotPartKeyID, &emptyBluetoothCharacteristicError{}, "failed to read robot part key ID", | ||
); robotPartKeyIDErr != nil { | ||
cancel() | ||
} | ||
}, | ||
wg.Done, | ||
) | ||
utils.ManagedGo( | ||
func() { | ||
if robotPartKey, robotPartKeyErr = retryCallbackOnExpectedError( | ||
ctx, bwp.readRobotPartKey, &emptyBluetoothCharacteristicError{}, "failed to read robot part key", | ||
); robotPartKeyErr != nil { | ||
cancel() | ||
} | ||
}, | ||
wg.Done, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pattern feels odd to me - why are we starting 4 goroutines that's each calling 1 function in a loop? Something useful for context would be to know on a high level how each of the functions will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify handling asynchronous Bluetooth reads, I created a function that calls readSsid
, readPsk
, readRobotPartKeyID
, and readRobotPartKey
in retrying loops (1x per second). Since these values can arrive at any time—or may never be set—we want to wait for each one in parallel.
Rather than leaving this logic to the caller, which could be error-prone, this function ensures all values are retrieved before returning. And in the error case, it cancels context for all. Each function checks an in-memory store for existing data.
The requiresWiFiCredentials and requiresCloudCredentials booleans add flexibility, allowing callers to request only WiFi credentials, cloud credentials, or both as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at https://github.com/viamrobotics/agent/pull/68/files so let me know if I'm missing something.
It seems possible to only have 1 goroutine that calls readSsid, readPsk, readRobotPartKeyID, and readRobotPartKey sequentially and then retry every second, is there a reason why not?
…move custom types that are now redundant.
"go.viam.com/utils" | ||
) | ||
|
||
// bluetoothWiFiProvisioner provides an interface for managing BLE (bluetooth-low-energy) peripheral advertisement on Linux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// bluetoothWiFiProvisioner provides an interface for managing BLE (bluetooth-low-energy) peripheral advertisement on Linux. | |
// bluetoothWiFiProvisioner provides methods for managing BLE (bluetooth-low-energy) peripheral advertisement on Linux. |
} | ||
v, err := fn() | ||
if err != nil { | ||
if errors.As(err, &expectedErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self to fix this and manually test the error matching works.
func retryCallbackOnExpectedError( | ||
ctx context.Context, fn func() (string, error), expectedErr error, description string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func retryCallbackOnExpectedError( | |
ctx context.Context, fn func() (string, error), expectedErr error, description string, | |
func retryCallbackOnExpectedError[T any]( | |
ctx context.Context, fn func() (T, error), expectedErr error, description string, |
@cheukt can't seem to get out of "review" mode on my PR which is preventing me from commenting on your comments.
No reason not to do that, other than we will have to add additional state in this function for "skipping" a read if we've already received a value for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for self.
ok, then we should prefer less goroutines - we can probably not spin a new one off at all and just use a for loop in the Wait function. I would avoid goroutines unless necessary, it introduces complexity and adds a ton of mental overhead for any future reader |
5f6a986
to
9377326
Compare
…unexported interface so that the BT functionality can be mocked from provisioning tests.
… methods that exist on its implementation from the calling code (in networkmanager.go).
… unimplemented and left for follow up PRs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is far from finished, but left some initial comments. (Apologies if you already know these things and just haven't gotten to them yet.)
|
||
// NewBluetoothWiFiProvisioningServiceLinux returns a service which accepts credentials over bluetooth to provision a robot and its WiFi connection. | ||
func NewBluetoothWiFiProvisioningService(ctx context.Context, logger logging.Logger, name string) (*bluetoothServiceLinux, error) { | ||
switch os := runtime.GOOS; os { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we don't want to do per-OS logic at runtime. Go has a pretty clever mechanism (build constraints) for making different versions of a file (and therefore the functions it contains) build only under certain conditions. E.g. the "windows" build would include only the windows code, and the regular/linux file would include the linux versions. That keeps code cleaner and resulting binaries smaller as well.
See https://pkg.go.dev/cmd/go#hdr-Build_constraints for details, but feel free to ask me for more examples as we've used it quite a bit in RDK itself.
} | ||
|
||
// Not ready to return (do not have the minimum required set of credentials), so sleep and try again. | ||
time.Sleep(time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use time.Sleep in agent/subsystems, it can cause health checks to fail. If you're inside a loop, use the mainLoopHealth/bgLoopHealth.Sleep(ctx) function instead
wg := sync.WaitGroup{} | ||
wg.Add(1) | ||
|
||
utils.ManagedGo(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you're backgrounding this, only to wg.Wait() right afterwards.
wg.Wait() | ||
|
||
return &userInput{ | ||
SSID: ssid, PSK: psk, PartID: robotPartKeyID, Secret: robotPartKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of individual variables to have be shared into the lambda. Why not just declare &userInput{} itself and set it's contents inside the lambda?
Summary
Add type (without implementing a low-level Linux solution) that can be "plugged-in" to the Agent provisioning flow for spinning up BT, waiting for credentials, and cleanly shutting itself down.
Included
The following functionality is added in this PR:
Example usage
The following code snippet describes how I plan to use the functionality added in this PR.