tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
3k stars 252 forks source link

Configure WiFi options from the UI - Menu #131

Closed mtlynch closed 1 month ago

mtlynch commented 4 years ago

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

mtlynch commented 10 months ago

Bumping priority as I recently received a request to enable this.

shalver-tp commented 3 months ago

This will likely be a popup that allows the user to input the needed information.

jotaen4tinypilot commented 3 months ago

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?

shalver-tp commented 3 months ago

@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.

jotaen4tinypilot commented 3 months ago

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.

UI

Here 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 state

When the user didn’t have a previous WiFi config.

Screenshot 2024-06-11 at 20 43 40

Invalid credentials / unsuccessful connection

When 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.

Screenshot 2024-06-11 at 21 03 40

Edit/Connected state

When the user entered a WiFi config, and the device is currently using it.

Screenshot 2024-06-11 at 21 46 45

Technical considerations

Reboot

It 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 scenarios

It currently looks like there are no major error scenarios that we have to consider. So far, I can think of the following:

Configuring WiFi programmatically

I’m not sure whether using the raspi-config nonint command (as suggested in our FAQ) is the best way for us to read or edit the WiFi settings programmatically. It seems that the raspi-config command (even in the nonint “non-interactive” version) is primarily meant to be invoked by humans, and it might not be powerful enough to support all operations we need. It might be better for us to go via the /etc/wpa_supplicant/wpa_supplicant.conf file directly, which is where the OS stores the WiFi settings.

Tasks

Based 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.

jdeanwallace commented 3 months ago

@jotaen4tinypilot - RE:

Find most suitable way to access and alter the WiFi configuration programmatically

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).

I’m not 100% certain whether configuring WiFi could have any potential side effects with other networking features. I’m thinking of the “Static IP” feature in particular, because I remember that there might have been some sort of implication there with regards to wireless connections.

Yes, currently enabling/disabling the WiFi using the above scripts nukes any Static IP config too: https://github.com/tiny-pilot/tinypilot/blob/7a31d856a6c2e5d8da3ef9c7f067a11f1b7a7e77/debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi-ap#L37-L39

jotaen4tinypilot commented 3 months ago

@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 /etc/wpa_supplicant/wpa_supplicant.conf, but e.g. we don’t have to touch /etc/dhcpcd.conf, so I’m not sure yet whether we would really conflict with the Static IP feature here. I haven’t fully reproduced what the raspi-config tool is doing, though, so I’m not 100% certain here yet.

shalver-tp commented 3 months ago

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)

jdeanwallace commented 3 months ago

Wouldn’t configuring the device to act as WiFi access point be something slightly different from this task, though?

Oh yes, you're right. I didn't register the "access point" part. My mistake.

jotaen4tinypilot commented 3 months ago

Okay, I’ve added a task list to the initial ticket description, which we can update as we move along.

jotaen4tinypilot commented 3 months ago

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.

jotaen4tinypilot commented 2 months ago

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.

shalver-tp commented 2 months ago

@cghague @db39 FYI about the fallback scenario upon first feature release. (see the recent comment)

jotaen4tinypilot commented 2 months ago

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.

https://github.com/tiny-pilot/tinypilot/assets/83721279/cb138881-1b77-40d3-863d-5bd6479d0efb

jotaen4tinypilot commented 2 months ago

I’ve split out subtask (f) to its own ticket, so that we can prioritize it separately (or defer it for now).

jotaen4tinypilot commented 2 months ago

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.

jotaen4tinypilot commented 1 month ago

This feature is completed now. Sub-task (f), the connection indicator, is tracked in its own ticket.