Skip to content

[APP-7946] Additional Bluetooth Provisioning Changes #96

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

Merged
merged 15 commits into from
Apr 16, 2025
Merged

Conversation

Otterverse
Copy link
Member

Further work to support the bluetooth provisioning method and integrate with the bluetooth socks proxy work.

@Otterverse Otterverse marked this pull request as ready for review April 14, 2025 22:12
Comment on lines +33 to +35
manufacturerKey = "manufacturer"
modelKey = "model"
fragmentKey = "fragment_id"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a change on my end to read the fragment id, should i read the manufacturer and model for any reason? The only thing I could think of was for displaying it in the UI in some way (from my perspective)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are purely informative for generic whitelabeling purposes. Safe to ignore, but I wanted to match parity with with web/grpc provisioning APIs.

Copy link
Member

@abe-winter abe-winter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did read-through to look for non-provisioning + windows issues -- one comment about startup time, no critical flags

@@ -13,6 +13,7 @@ SERVICE_FILE=$(cat <<EOF
[Unit]
Description=Viam Services Agent
After=NetworkManager.service
After=bluetooth.service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking through consequences of the After

systemd docs for After directive

it sounds like this is only respected if 'both units are being started' (i.e. systems without bluetooth will still be able to start agent)

note this has potential to delay startup on systems which have bluetooth -- this is not a merge blocker, but consider pinging SEs before actually releasing this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it used to be "Wants=NetworkManager" and that caused problems for systems without. After means "if you're starting it anyway, sequence."

I did a couple timing tests, bluetooth starts up in basically the same second as NetworkManager on my Pi (same timestamp) so I don't think the delay is an issue. And it's definitely an issue if bluetooth provisioning fails to work because it started up in the wrong order. Will definitely let SE know anyway.

@Otterverse Otterverse requested a review from benjirewis April 16, 2025 17:02
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective looks great!

return errw.Wrap(err, "registering custom agent")
}

call = obj.Call("org.bluez.AgentManager1.RequestDefaultAgent", 0, dbus.ObjectPath(BluezAgentPath))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] Any reason to make your NoInputNoOutput agent the default agent on the device? Were you having issues without this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normal "register agent" functions in other libraries all do this. The idea was maybe I could hook the incoming requests from the pop-up. Otherwise, the registered agent is supposed to ONLY deal with pairing requests our app initiates (and we don't initiate pairing requests ourselves.) Feel free to play around with it.

@Otterverse Otterverse merged commit c2fc5c4 into main Apr 16, 2025
3 checks passed
@Otterverse Otterverse deleted the james-btfixes branch April 16, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants