Skip to content

Update enable-wifi to read PSK input from stdin#1870

Merged
db39 merged 5 commits intomasterfrom
1869-update-enable-wifi-to-read-psk-input-from-stdin
Mar 6, 2025
Merged

Update enable-wifi to read PSK input from stdin#1870
db39 merged 5 commits intomasterfrom
1869-update-enable-wifi-to-read-psk-input-from-stdin

Conversation

@db39
Copy link
Copy Markdown
Contributor

@db39 db39 commented Mar 6, 2025

Resolves https://github.com/tiny-pilot/tinypilot-pro/issues/1464

This change updates the enable-wifi script to read the PSK from stdin instead of using an argument.

We also update the enable_wifi function in network.py to use the new stdin interface.

Review on CodeApprove

@db39 db39 linked an issue Mar 6, 2025 that may be closed by this pull request
@db39 db39 requested a review from jdeanwallace March 6, 2025 12:04
Copy link
Copy Markdown
Contributor Author

db39 commented Mar 6, 2025

Automated comment from CodeApprove ➜

@jdeanwallace please review this Pull Request

Copy link
Copy Markdown
Contributor

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved

LGTM!


In: Discussion
The code looks good and seems to edit /etc/wpa_supplicant/wpa_supplicant.conf correctly. However, I'm having some issue connecting to my own wifi network at home when specifying a password, but I've had issues before.

Can you confirm that you're able to connect to your network when a password is specified?


In: app/network.py:

> Line 163
            process.communicate(input=wifi_settings.psk)

It's technically not correct that we always specify a stdin pipe, even when there isn't a password. Can we rather do something like this?

if wifi_settings.psk:
    args.append('--psk')
    subprocess.Popen(args, stdin=subprocess.PIPE, text=True).communicate(input=wifi_settings.psk)
else:
    subprocess.Popen(args)

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 16
                      The password must be 8-63 characters in length.

Can we specify that this is the password used for authenticating with the wifi network? Suggestion:

Optional. Authenticate with the WiFi network using a password read from stdin. The password must be 8-63 characters in length.


👀 @db39 it's your turn please take a look

Copy link
Copy Markdown
Contributor Author

@db39 db39 left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

In: Discussion

Can you confirm that you're able to connect to your network when a password is specified?

Yes - I'm able to connect to my Wi-Fi network using a password:

image.png


In: app/network.py:

> Line 162
            process.communicate(input=wifi_settings.psk)

Resolved


In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:
Resolved

Copy link
Copy Markdown
Contributor

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

@jdeanwallace jdeanwallace dismissed their stale review March 6, 2025 15:42

Review approved on CodeApprove

@db39 db39 merged commit 324392c into master Mar 6, 2025
@db39 db39 deleted the 1869-update-enable-wifi-to-read-psk-input-from-stdin branch March 6, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update enable-wifi to read PSK input from stdin

2 participants