Configure WiFi options from the UI - Menu #131
Comments
|
Bumping priority as I recently received a request to enable this. |
scott-tp
changed the title
|
This will likely be a popup that allows the user to input the needed information. |
|
Just a quick question as I’m starting to look into this: we want this to be a Community feature, right, not a Pro-exclusive one? |
|
@jotaen4tinypilot That's a good question! Let's make it a Community feature as I think a UI change like this makes sense to go across both versions. It isn't really a new "feature" that we would want to wall off. |
|
I’ve started to look into this ticket and wanted to discuss some initial thoughts, to make sure this is going into the right direction. UIHere are some mockups of how the UI of the WiFi dialog could look. The mockups are not meant to be pixel-perfect, but they should rather give an initial impression of the overall look-and-feel of the feature. Initial/Empty stateWhen the user didn’t have a previous WiFi config. 
Invalid credentials / unsuccessful connectionWhen the user entered a WiFi config, but the device wasn’t able to connect – e.g., due to invalid credentials, or the wireless network being unreachable. 
Edit/Connected stateWhen the user entered a WiFi config, and the device is currently using it. 
Technical considerationsRebootIt seems that the default way to effectuate WiFi settings on a RaspberryPi device requires a system reboot. While we already know how to implement that (e.g., in the “Hostname” dialog), we typically try to avoid reboots: they are inconvenient for the user as they take a while (like 20–30 seconds), and they also always carry the subtle risk that something goes wrong – e.g. that the UI cannot re-establish the connection to the backend automatically, which either results in an error message, or in an eternal loading spinner. There seem to be alternative options to apply WiFi settings programmatically on the RaspberryPi which don’t require a system reboot, so I suggest to research this and see whether we find a good way that’s reliable enough for our purposes. Potential error scenariosIt currently looks like there are no major error scenarios that we have to consider. So far, I can think of the following:
Configuring WiFi programmaticallyI’m not sure whether using the TasksBased on the discussion above, I’d suggest the following task break down:
All in all, I’d estimate this feature as “large”, and I currently wouldn’t expect any major complications. |
|
@jotaen4tinypilot - RE:
We have 2 existing scripts that enable/disable the WiFi access point, namely: From what I can see, they might not require a reboot (haven't tested).
Yes, currently enabling/disabling the WiFi using the above scripts nukes any Static IP config too: tinypilot/debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap Lines 37 to 39 in 7a31d85 |
|
@jdeanwallace thanks for chiming in! Wouldn’t configuring the device to act as WiFi access point be something slightly different from this task, though? My reading was that this task is about configuring the device so that it connects wirelessly to an existing WiFi network, whereas setting up a WiFi access point would be about configuring the device so that it broadcasts it’s own private WiFi network, independent of any existing WiFi network. For just storing the WiFi connection parameters, I think we primarily have to touch |
|
We had a bit of discussion related to wifi and static ips recently here: https://github.com/tiny-pilot/tinypilot-pro/issues/1340 Yes, this is more about getting to a dual NIC setup (Wi-Fi and wired at this point) |
Oh yes, you're right. I didn't register the "access point" part. My mistake. |
|
Okay, I’ve added a task list to the initial ticket description, which we can update as we move along. |
|
Brief intermediate update: regarding sub-tasks (a) and (b), I’ve set up a draft PR containing two privileged scripts that would seem viable and that would address both requirements at once. I’ve asked Charles for some initial feedback, to validate/double-check the basic approach. |
|
Re the “Potential error scenarios”, I realized that there is an additional error scenario: if the user is on a WiFi connection and no Ethernet cable is plugged in, and the user then either changes or disables the WiFi configuration, they’d effectively lose access to their device. The “fix” would be to plug in an Ethernet cable again, and change or reenable the WiFi configuration. We already had a similar problem with the “Static IP” feature, where the device theoretically could have lost network access if the router didn’t accept the desired static IP. Our solution there was to have a device-side logic, that would check whether the device has become unreachable, and then automatically revert the static IP settings. We can do something similar for WiFi, although I’m not sure we really need that – my thinking is this:
So my suggestion would be to go with the warning for now. If we’d see that this becomes an issue frequently, we could still consider a more sophisticated prevention mechanism, and add that later on. |
|
Just a quick work-in-progress update: I’ve created a draft PR stack (for the underlying commands, the backend logic, and the new dialog in the UI). The code/work of the PRs is not complete yet, but I was already able to successfully verify the entire flow of the WiFi feature on my device. Screen.Recording.2024-06-25.at.20.59.51.mov |
|
I’ve split out subtask (f) to its own ticket, so that we can prioritize it separately (or defer it for now). |
Related #131, subtask (g). As the WiFi-AP scripts [overwrite the `wpa_supplicant.conf` file](https://github.com/tiny-pilot/tinypilot/blob/6f37e68acbb729d82b76ed0dad0f67763926f29d/debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap#L160-L172), we have to extend the note in the help text that running the scripts will clear any WiFi settings. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1817"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> Co-authored-by: Jan Heuermann <[email protected]>
Related #131, parts (a) and (b). This PR adds two scripts for enabling and disabling the WiFi network interface on the device, so that you can also access TinyPilot through a wireless network connection. I already discussed the basic approach last week with Charles (see comment and discussion below), because of his familiarity with networking/WiFi. Since then, I did more testing in various scenarios, and also already opened two subsequent draft PRs for the [backend](#1805) and [frontend](#1810). They are WIP still, but they may give a rough impression of how the WiFi feature will look in its entirety. Overall, it seems to be looking good, so I thought it might make sense to already start the review process. A few notes on the implementation: - We write into the `wpa_supplicants.conf` file directly to configure the WiFi connection, and we use the marker-section mechanism to manage the TinyPilot-specific part of the configuration. Depending on whether the WiFi network is password-protected or not, we either use the `wpa_passphrase` command, or we generate the config section manually. - I’m not sure it’s worth to validate the `--country` input value in the `enable-wifi` script: I’d plan to do validation in the backend later on, and the network service seems to behave graceful against invalid country codes. For the `--psk` flag, we have to do a basic length validation, because otherwise the `wpa_passphrase` command would fail. - For testing on device, you can use the `ifconfig` command to check whether the `wlan` network interface is up and active. (I.e., whether `ifconfig` outputs a `wlan` block, and whether that has a `inet`/`inet6` address assigned to it.) --- ### Original draft PR comment (2024-06-13) This PR is a draft for now, because I wanted to evaluate the technical approach of the underlying WiFi-related logic first, before investing time into the implementation of the application logic (i.e., the Python part and the UI). I saw that you @cghague have been involved into [our WiFi FAQ document](https://github.com/tiny-pilot/tinypilotkvm.com/blob/46696cc1698e1eb73051616423ab3c6b09a4f5e1/content/faq/enable-wifi/index.md), so I was hoping that you are maybe familiar with the technical domain. Could you look over the privileged scripts of this PR and give me some initial, high-level feedback about whether the proposed approach seems sensible to you? At this point, I’d mainly like to avoid going into a wrong direction, or to miss any potential caveats or other issues I may have forgotten to consider. A few notes on the implementation: - For the purposes of #131, we need to fully automate the logic for enabling and disabling WiFi, so that we can invoke both procedures from the Python backend programmatically. The `enable-wifi` script obviously would have to parse CLI arguments to parametrize the SSID, PSK, and country code. The `disable-wifi` script wouldn’t take any arguments. - I’ve successfully tested the scripts on my device, and so far they appear to work fine. By issuing `rfkill unblock wifi` and `wpa_cli -i wlan0 reconfigure`, it seems to be possible to effectuate the changes and activate WiFi without having to reboot the device. This would make things easier for us in the UI, but it would also be more convenient and snappier for the end-user. - My thinking was that it doesn’t make sense to use the `raspi-config` tool in the scripts, because that seems to be too limited. For example, we don’t want to just keep adding new WiFi configs to the `wpa_supplicant.conf` file, but we basically want to maintain *one* “TinyPilot-owned” custom WiFi config, which we then also can delete or modify in a targeted way. For this, it seemed more sensible to me to write into the `wpa_supplicant.conf` file directly, using the [marker-section technique](https://github.com/tiny-pilot/tinypilot/blob/9bbc056c6e4dcec1804b99b3cdcbd0bcdf1ca0dc/debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh) that we already employ elsewhere. The downside is that this requires more low-levelled logic on our side. If you’d generally agree with the proposed approach and underlying reasoning, I’d proceed to wrap up this PR and to build the surrounding application logic. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1804"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
Related #131, part (b). Stacked on #1804. I realized that we need to add another privileged script for reading files with marker sections, because we are unable to read those directly via the Python process in case those files are owned by the `root` user. Usage is demonstrated in [the subsequent backend PR](#1812); it’s basically … ```bash /opt/tinypilot-privileged/scripts/print-marker-sections /path/to/file.txt ``` … where the entire invocation has to be unblocked in the sudoers file. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1811"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
Related #131, part (c). Stacked on #1811. This PR adds the backend logic and endpoints that we need for the WiFi dialog. - There are endpoints for reading, writing, and deleting the WiFi configuration. There is also one endpoint for checking the status of the network connectivity, which allows us to display warnings or status indicators in the frontend. - As [mentioned in the code comment](https://github.com/tiny-pilot/tinypilot/pull/1812/files#diff-43d8494a595e7d1f838bc01d5f2c6b18eebf61ca63bc3bbf2837dbcb27a8e109R62-R63), we cannot read the `wpa_supplicant.conf` file directly due to file ownership. Therefore, we use the [new `print-marker-sections` privileged script](#1811) from the previous PR. - The scripts for enabling and disabling are executed in a “fire and forget” manner, otherwise it may happen that the frontend requests hang due to e.g. a change/interruption in the network (which would result in an eternal loading spinner). As we only write a config file and trigger service/daemon restarts, we wouldn’t get direct feedback about errors anyway, but we’d have to poll for the resulting status, which can take quite long, though. - Note that we also can’t really verify the WiFi credentials, because the desired WiFi might not be reachable at the time where the user wants to enter the settings (e.g., because they want to enter the config ahead of time). - The validation of the incoming request data is relatively important, because we will show the error messages in the frontend for input validation. So we need to make sure that we detect all relevant potential issues. The validation errors would be user-facing. This PR can probably be tested best via the [subsequent frontend PR](#1813) – that one isn’t 100% code-complete yet, but it’s pretty close in terms of functionality. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1812"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
|
fyi, as we are closed to finishing the feature, I’ve already merged the bottom of the PR stack with the backend changes, to contain the scope of pending changes. |
Related #131, part (d). Stacked on #1812. This PR adds a new dialog “Wi-Fi” to the “System→Network” menu, which makes the Wi-Fi feature available to end-users. Summary of the discussion below: - We made the wording so that it’s clear that we only store the WiFi credentials, but we don’t verify whether they are actually accepted by the WiFi router, or whether the WiFi is reachable at all. For further clarification, we also added a success state to the dialog. - In a subsequent PR, we’ll turn the “country code” free text field into a dropdown, to make it easier for users to enter their correct and applicable value. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1813"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
Related #131, part (h). Stacked on #1813. We didn’t have a `<select>` field in the style guide yet, and we also were missing the general CSS for it. We’ll need this for [turning the “Country Code” input field into a select-field](#1820), as [discussed in the previous PR](#1813 (comment)). Once we merged this into Pro, we can also refactor [the inlined CSS of the `<select>` field in the static IP dialog](https://github.com/tiny-pilot/tinypilot-pro/blob/f806639d5124e7edad8b9a44a727ecf49f73ed54/app/templates/custom-elements/static-ip-dialog.html#L38-L46). <img width="840" alt="Screenshot 2024-07-05 at 08 34 39" src="https://github.com/tiny-pilot/tinypilot/assets/83721279/c49b6a94-d114-495b-9dcb-ba32eb30029d"> <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1819"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
Related #131, part (h). Stacked on #1819. This PR turns the free text input field for the country code in the WiFi dialog into a `<select>` dropdown, to make it easier for users to choose their correct and applicable country code value. <img width="811" alt="Screenshot 2024-07-05 at 08 21 33" src="https://github.com/tiny-pilot/tinypilot/assets/83721279/34b6469b-3681-4545-af26-1e6a567c4756"> A few notes: - I thought the simplest approach to retrieve the available country codes was by copying them over from the `/usr/share/zoneinfo/iso3166.tab` file. An alternative would be to parse the file in the backend, and retrieve the list dynamically via an API endpoint. That would then also allow us to validate the input value against this list in the request parser. However, I think the cost<>benefit ratio of such an implementation would be questionable, especially considering that [the ISO definitions don’t seem to change very often](http://www.statoids.com/w3166his.html). - I adjusted the input field label from “Country Code” to “Country”. I still suggest to display the country codes in the options (e.g., `United States (US)`) for informational reasons, to satisfy both technical and non-technical users. - I removed the `size` attribute of the `<input>` field in favor of a `width`, to make the sizing match the one of the `<select>` field. The latter has slightly different box sizing behaviour, so it’s width differs by `1em`. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1820"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
|
This feature is completed now. Sub-task (f), the connection indicator, is tracked in its own ticket. |
TinyPilot users can currently configure their devices for WiFi via the command-line, but it would be good if we offered a path to do it right from the web UI.
Tasks
/usr/share/zoneinfo/iso3166.tabThe text was updated successfully, but these errors were encountered: