Skip to content
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

WiFi dialog: add backend endpoints and logic #1812

Merged
merged 35 commits into from
Jul 5, 2024

Conversation

jotaen4tinypilot
Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot commented Jul 1, 2024

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, we cannot read the wpa_supplicant.conf file directly due to file ownership. Therefore, we use the new print-marker-sections privileged script 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 – that one isn’t 100% code-complete yet, but it’s pretty close in terms of functionality.
Review on CodeApprove

@jotaen4tinypilot
Copy link
Contributor Author

Same CircleCI issue here…

Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jdeanwallace please review this Pull Request

Copy link
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 (8 unresolved comments)
Approval will be granted automatically when all comments are resolved

LGTM!


In: app/api.py:

> Line 204
@api_blueprint.route('/network', methods=['GET'])

Can we distinguish the network status endpoint from the network settings endpoint by adding an extra /status and /settings section to the path? For example:

  • GET /api/network/status
  • GET /api/network/settings/wifi
  • PUT /api/network/settings/wifi
  • DELETE /api/network/settings/wifi

In: app/network.py:

> Line 5
_CONFIG_FILE = '/etc/wpa_supplicant/wpa_supplicant.conf'

This variable is unused. Can we remove it?


In: app/network.py:

> Line 25
class WiFiSettings:

(nit) In terms of class naming conventions, what do you think of adopting this rule for python?

So WiFiSettings would become WifiSettings.

I was initially under the impression that this was already part of our style guide, but it's not.


In: app/network.py:

> Line 74
    for line in config_lines.split('\n'):

We could also use str.splitlines.


In: app/network.py:

> Line 93
        wifi_settings: The new, desired settings.

Can we mention that wifi_settings is of type WiFiSettings?


In: app/network.py:

> Line 105
        return subprocess.Popen(args)

Can we mention the return type in the docstring? Also, seeing as this is "fire and forget", should we even return anything?

Same for disable_wifi.


In: app/request_parsers/network_test.py:

> Line 54
            network.parse_wifi_settings(

Is this point in the code reachable? We might need subTest or assertRaises here.


In: app/request_parsers/network.py:

> Line 24
        raise errors.InvalidWifiSettings('The country code is not a string.')

Can the error message mention what is expected? For example:

The country code must be a string.


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

Copy link
Contributor Author

@jotaen4tinypilot jotaen4tinypilot 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: app/api.py:
Sure, done.


In: app/network.py:
Ah, that was a left-over. Removed.


In: app/network.py:
I think the return can go away, at least we don’t actually need it.


In: app/network.py:
Resolved


In: app/network.py:
Resolved


In: app/network.py:
I’m not actually sure what the correct camel-case of WiFi would be, as I think the official spelling is Wi-Fi. In any event, I think WifiSettings might be less complicated to manage, so to me that would be reason enough to change it to that.


In: app/request_parsers/network_test.py:

> Line 55
            network.parse_wifi_settings(

Oh, no it wasn’t reachable, but we need separate with self.assertRaises like in the other test cases. Fixed.


In: app/request_parsers/network.py:
Resolved

@jdeanwallace jdeanwallace dismissed their stale review July 3, 2024 12:54

Review approved on CodeApprove

Copy link
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.

jotaen4tinypilot added a commit that referenced this pull request Jul 5, 2024
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]>
Base automatically changed from wifi-dialog/2-print-marker to master July 5, 2024 05:48
# Conflicts:
#	debian-pkg/etc/sudoers.d/tinypilot
#	dev-scripts/mock-scripts/print-marker-sections
@jotaen4tinypilot jotaen4tinypilot merged commit 2159320 into master Jul 5, 2024
13 checks passed
@jotaen4tinypilot jotaen4tinypilot deleted the wifi-dialog/3-backend branch July 5, 2024 05:52
jotaen4tinypilot added a commit that referenced this pull request Jul 16, 2024
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]>
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.

3 participants