Static IP address scripts Bookworm compatibility

[db39]

Resolves #1680

This change updates our static IP scripts to conditionally use nmcli for compatibility with Raspberry Pi Bookworm, retaining the previous implementation for Bullseye.

[db39]

@jdeanwallace - I've taken a stab at this to introduce Bookworm compatibility with our static IP scripts. I tested these scripts on Bookworm 64-bit and they seem to work as expected and don't require a reboot.

The one caveat with nmcli is that it requires the connection name, rather than the interface (i.e., 'Wired connection 1' instead of 'eth0'), so it's a little awkward. I think this is ok considering static IP changes only affect the Ethernet interface.

[db39]

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

[jdeanwallace]

Automated comment from CodeApprove ➜

Looks good. I'll test it on device in the next review iteration.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 134
  if [[ "${OS_CODENAME}" == "bullseye" ]]; then

Can we use single quotes for strings that use variable interpolation?

Same throughout.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 137
    readonly INTERFACE="Wired connection 1"

It might be misleading if an interface was specified but it's being treated as a connection name. Can we search for the correct connection name instead? Something like:

CONN_NAME="$(nmcli --fields NAME,DEVICE --terse connection show |
  grep --max-count 1 "${INTERFACE}" |
  cut --fields 1 --delimiter ':')"

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 137
    readonly INTERFACE="Wired connection 1"

Actually, if we're going to be moving to nmcli, then the --interface command-line option will probably need to be phased out.

Let's just ignore the interface when using nmcli and hard code the connection name to Wired connection 1. We're already hard coding it in the unset/apply scripts.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 145
OS_CODENAME="$(grep '^VERSION_CODENAME' /etc/os-release | cut --delimiter '=' --fields 2)"

We can also use lsb_release --codename --short to retrieve the distribution name.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 145
OS_CODENAME="$(grep '^VERSION_CODENAME' /etc/os-release | cut --delimiter '=' --fields 2)"

Also we can consider matching on the release number instead. Something like this:
https://github.com/tiny-pilot/tinypilot-pro/blob/ef880501bb51819de660ccff1b5d65b7e8d11fef/bundler/bundle/install#L34-L35

---

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

[db39]

Automated comment from CodeApprove ➜

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Matching by version number probably makes more sense if it's how we've checked this kind of thing in the past.

---

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

[jdeanwallace]

Automated comment from CodeApprove ➜

@db39 - I'm concerned about how these changes would affect the Pro repo where we have a slightly more complex procedure in place for setting a static IP via the Web UI (i.e., setting a temporary static IP, waiting for the new settings to be confirmed, then permanently persisting then new static IP) to avoid the user being locked out of their device.

Would you mind exploring the changes that would need to be made to the Pro repo in a new PR before we continue with merging these changes into the Community repo?

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 141
OS_VERSION="$(lsb_release --release --short)"

Can we mark this variable as readonly?

Same throughout.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 143
if [[ "${OS_VERSION}" == 11 ]]; then

Can we use double parentheses for this numeric comparison?

• https://github.com/tiny-pilot/style-guides/#arithmetic--numerical-comparisons

Same throughout.

---

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

[db39]

@jdeanwallace - Sure, I'll take a look at the Pro versions of the scripts and see what we'd need to do on that front.

[db39]

Automated comment from CodeApprove ➜

---

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved

[db39]

@jdeanwallace - looking at this some more, we might be able to add to nmcli's connections to configure a static IP instead of using modify:

sudo nmcli connection add type ethernet con-name tinypilot-static-ip ifname 'eth0' ip4 192.168.178.100 gw4 192.168.178.1

Then run `nmcli connection show:

NAME                 UUID                                  TYPE      DEVICE 
Wired connection 1   cf15d7fb-18ce-3d4e-a433-d6695b881897  ethernet  eth0   
lo                   230c75e5-4c72-417e-bb0d-79df205005cc  loopback  lo     
preconfigured        13d94203-335a-4ba4-bc57-5109f5f86a80  wifi      --     
tinypilot-static-ip  0704fd50-2c28-4c34-bd09-34d3b5812d17  ethernet  --   

Then you can switch between connections like this:

nmcli connection up tinypilot-static-ip

nmcli connection up "Wired connection 1"

This way, you don't have to save the previous config, you just switch between the connections. Although I'm not sure if this impacts other networking we do?

If we have to use nmcli modify and save the config, we could capture the config with:

nmcli connection show "Wired connection 1"

And output the fields we're interested in to a text file. Then we can restore the config from the file if required.

So the flow in Pro can be the same as it is currently as far as I understand (we'd just need to add nmcli to the additional steps).

[jdeanwallace]

@db39 - That's an interesting idea and it might work, but I'm not too sure how it will all fit together. Seeing as the tinypilot-pro repo has the more complex use-case (i.e., set temporary settings then revert or save settings permanently), can we create a draft PR in the Pro repo with an example of what you're suggesting or some version of it? That way it will be easier to test and give feedback on.

[db39]

@jdeanwallace - after more digging, creating a new connection could work, but if the user wants to change from one static IP to another, we would have to create an additional static IP connection. At that point, we'd have to store the details of the previous connection anyway (though maybe only the name), and we'd probably have to clean up the old connection. It also appears as though the default Wired connection 1 connection may get deleted if it's not being used.

It seems like following a similar flow to what we have already with dhcpcd (storing the config in a file) and just modifying Wired connection 1 instead might be the simplest option. I'll create a draft PR for that.

[db39]

It's been a little while since I last looked / worked on this.

Coming back to this, I was thinking about why we need to make the static IP script compatible with both Bullseye and Bookworm (particularly on Pro). It makes some sense with community because users may attempt to install TinyPilot Community on Bookworm and it would be nice to support that. But on Pro we're currently Bullseye-only, and when we do support Bookworm, won't we require users to re-flash to upgrade (like we did from Buster -> Bullseye)? Am I misunderstanding or missing something here?

[jdeanwallace]

why we need to make the static IP script compatible with both Bullseye and Bookworm (particularly on Pro)

@db39 - I think the confusion is that (as far as I know) we're not dropping support for Bullseye and we're only adding support for Bookworm. So users will still be able to install TinyPilot (Community or Pro) on both Bullseye and Bookworm via the command-line (at least).

Also, conditionally supporting both Bullseye and Bookworm allows us slowly test Bookworm compatibility on the whole system before rolling it out to our users. For example:

if BOOKWORM:
  do it the NetworkManager way
else:
  do it the dhcpcd way

[db39]

(as far as I know) we're not dropping support for Bullseye and we're only adding support for Bookworm.

Ahh, thanks! It would make sense if that's the plan. Do we have confirmation that the plan is to support both versions (@scott-tp)?