TinyPilot WiFi access point scripts #1778
Conversation
jdeanwallace
changed the title
jdeanwallace
changed the title
Automated comment from CodeApprove ➜⏳ @jotaen4tinypilot please review this Pull Request |
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 48
;;
We want defaults for SSID and country, but we should provide flags that allow the user to override them.
👀 @jdeanwallace, @jotaen4tinypilot it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 55
;;
Oh right, I forgot about that. Thanks.
👀 @jotaen4tinypilot, @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Could we add a little more commentary to the scripts? Especially for enable-wifi-ap, I think that might help to follow the structure and purpose of the code sections.
In: debian-pkg/opt/tinypilot-privileged/scripts/disable-wifi-ap:
Should we provide help output for this script as well?
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 31
--ssid NETWORK_NAME The network name. Defaults to: ${NETWORK_NAME}
Shall we say Optional. here at the beginning of the description text?
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
Is the blank line after this comment on purpose? (ditto below)
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 82
# Validate command-line arguments.
Do we need to add a check here for the NETWORK_COUNTRY value? (E.g., whether it’s set, or whether it’s two letters.)
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 95
NETWORK_AP_IP_ADDRESS='192.168.50.1'
Is it possible to make these readonly?
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 106
"${SCRIPT_DIR}/strip-marker-sections" /etc/dhcpcd.conf
Would this clear any potential static IP setting that might be present in the config file? If so, would this be something we should account for in some way? (I’m thinking, in the most simple case, we could point it out in the help output or in a code comment, just to “document” that we have considered this case.)
👀 @jdeanwallace, @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
I'm actually not 100% sure what's going on in these scripts... but I can find out and add comments.
In: debian-pkg/opt/tinypilot-privileged/scripts/disable-wifi-ap:
Done.
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
Done.
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
Done.
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
Yes it was, with the intention of it being a section divider (because the comment applies to the next several lines of code).
But if it sparks slight confusion, I would rather default to a normal comment instead. Fixed.
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 88
# Validate command-line arguments.
Done.
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 118
"${SCRIPT_DIR}/strip-marker-sections" /etc/dhcpcd.conf
Yes, that's correct. I've added a note to the help output. Thanks!
👀 @jotaen4tinypilot, @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
⏳ Approval Pending (2 unresolved comments)
Approval will be granted automatically when all comments are resolved
Cool, this looks good. I also tested on device.
In: Discussion
Ah alright. I think it would be cool to eventually have more commentary in both scripts, to make it easier to understand what’s happening, and also to make them match our usual commenting style in bash scripts. Considering that the code was already there before, though, would it make sense to merge this PR as is for now, and to create a separate follow-up ticket where we loop in Dave to help us with adding good comments? (Dave appears to have written the FAQ page.)
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
> Line 94
if [[ -z "${NETWORK_PASSWORD}" ]]; then
It seems that the hostapd service requires the WPA passphrase to be 8–63 characters in length, at least it appears to fail to restart otherwise: (from the systemd logs, after trying asdf as password)
tinypilot hostapd[2170]: Configuration file: /etc/hostapd/hostapd.conf
tinypilot hostapd[2170]: Line 10: invalid WPA passphrase length 4 (expected 8..63)
Shall we validate the password against this rule?
👀 @jdeanwallace, @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Good idea! Tracking in #1784
In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap:
Ah good catch, thanks.
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
Related #1711
This PR converts the individual code snippets from our blog post on how to host a WiFi access point on your TinyPilot device, into less intimidating scripts:
enable-wifi-apdisable-wifi-apNotes: