Skip to content

add: Cellular Network support (rebase of #2765) - #3096

Open
kmf wants to merge 8 commits into
AvengeMedia:masterfrom
kmf:pr-2765-cellular-rebase
Open

add: Cellular Network support (rebase of #2765)#3096
kmf wants to merge 8 commits into
AvengeMedia:masterfrom
kmf:pr-2765-cellular-rebase

Conversation

@kmf

@kmf kmf commented Aug 17, 2026

Copy link
Copy Markdown

Description

Rebased and review-fixed version of #2765 (cellular network support).

Original work by @yechielw. Rebased onto current master and tested on a machine with a Fibocom L831-EAU-00 (Vodacom LTE).

Review from @bbedward on #2765:

  • Rebased onto master (term_freeze.json is gone)
  • Incremented APIVersion 30 → 31
  • Regenerated settings_search_index.json via extract_settings_index.py
  • Reused existing translation terms where they already cover the new UI
  • Moved the Cellular settings tab to index 46 (avoids Battery / Mouse & Touchpad collisions)
  • Added wpa_supplicant cellular stubs so the Backend interface still compiles
  • Kept the newer hotspot Control Center UI while adding cellular

Fixes found while testing:

  • Do not assign the readonly userPreference alias (Cellular chip was a no-op)
  • Make the modem row actually clickable
  • If no GSM profile exists, AddAndActivateConnection like Ethernet connect

Type of change

  • New feature (non-breaking change that adds functionality)

Related issues

Continues / supersedes #2765

Checklist

  • Code follows CONTRIBUTING.md
  • New user-facing strings use I18n.tr(), reusing existing terms where possible
  • Go: go test ./internal/server/... passed
  • Tested on hardware with a WWAN modem

yechielw and others added 6 commits August 17, 2026 12:42
Bump the network API version, drop the deleted term freeze file,
regenerate the settings search index, and reuse existing translation
terms where they already cover the new cellular UI.
Assigning SettingsData.networkPreference through the readonly alias
threw on click, so the Cellular segment in Control Center did nothing.
The Control Center type switcher was bound only to preference, so
Cellular snapped back to Ethernet. The settings modem row highlighted
on hover but had no click handler. Show the modem as a connect target
when no APN profiles exist.
Clicking Mobile broadband only activated an existing profile, so a
modem with no APN did nothing useful. Fall back to AddAndActivate
like ethernet connect.
@kmf kmf mentioned this pull request Aug 17, 2026
12 tasks
}
}

b.cellularDevices[iface] = &cellularDeviceInfo{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This map access needs to be protected. Either uses the syncmap from dankgo or protect with RW mutex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e922cc9. Added cellularDevicesSnapshot() / cellularDeviceByIface() / setCellularDeviceInfo() / removeCellularDeviceByPath() guarded by the existing devMutex, mirroring the ethernet and wifi accessors already in this file — cellularDevices is now covered by the same pattern rather than a second mechanism. Every raw access is converted: Initialize, both signal-pump loops, handleDeviceAdded, handleDeviceRemoved, updateCellularState, updateAllCellularDevices and DisconnectCellularDevice. Also updated the devMutex doc comment to mention the map.

Happy to switch to syncmap instead if you would rather the new map not extend devMutex.

// CancelCredentials provides a mock function for the type MockBackend
func (_mock *MockBackend) CancelCredentials(token string) error {
ret := _mock.Called(token)
// ActivateCellularConnection provides a mock function with given fields: uuid

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file shouldnt be hand-edited its generated. Re-generate mocks with mockery v3 just by running mockery e.g. go run github.com/vektra/mockery/v3@latest

This info about mockery was missing from CONTRIBUTING.md but I just added it on master

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e922cc9 — regenerated with go run github.com/vektra/mockery/v3@latest (v3.7.3) instead of hand-editing. The file is now purely additive against master (395 insertions, 0 deletions); the hand-edited version had drifted from the generator output.

One note: mockery also regenerates internal/mocks/internal/plugins/mock_GitClient.go, which is stale on master and unrelated to cellular. I reverted that one to keep this PR focused — worth a separate regen commit on master.

StyledText {
width: parent.width
visible: (NetworkService.cellularDevices?.length ?? 0) === 0 && cellularConnectionsModel.values.length === 0
text: I18n.tr("No devices found")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this text could render twice when theres no modem and no saved config (line 632 also)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, confirmed. The label at the top of cellularColumn was gated only on cellularDevices.length === 0, while this one also checks the connections model — so with no modem and no saved config both were visible. Dropped the top one in e922cc9; this one's condition was already the correct combined check.

}

func (b *NetworkManagerBackend) refreshCellularState() {
b.updateAllCellularDevices()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-use this method in more places, some places are repeating this logic

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e922cc9. refreshCellularState() is now used in SetCellularEnabled, DisconnectCellular, DisconnectCellularDevice, ActivateCellularConnection and the device-removal path in backend_networkmanager_signals.go.

I left the remaining sites in signals.go alone on purpose — those interleave ethernet/wifi/hotspot updates and fire a single onStateChange at the end, so folding the helper in would emit an extra mid-sequence broadcast.

if _, err := nm.AddAndActivateConnection(settings, dev); err != nil {
delete(settings["gsm"], "auto-config")
if _, retryErr := nm.AddAndActivateConnection(settings, dev); retryErr != nil {
return fmt.Errorf("failed to create and activate cellular connection: %w", err)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should wrap retryErr, seems like it'd be more indicative of the real error

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — fixed in e922cc9, it now wraps retryErr. Also added a short comment noting the retry exists because older NetworkManager/ModemManager stacks reject gsm.auto-config.

width: enableCellularLabel.implicitWidth + Theme.spacingL * 2
height: enableCellularLabel.implicitHeight + Theme.spacingM * 2
radius: height / 2
color: enableCellularButton.containsMouse ? Qt.rgba(Theme.primary.r, Theme.primary.g, Theme.primary.b, 0.12) : Qt.rgba(Theme.primary.r, Theme.primary.g, Theme.primary.b, 0.08)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hand rolling Qt.rgba re-use the appropriate tokens like wifiOffContent does Theme.surfaceTextSecondary/primaryHover/primaryHoverLight

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e922cc9 — the icon now uses Theme.surfaceTextSecondary and the button uses Theme.primaryHover / Theme.primaryHoverLight, matching wifiOffContent.

- Guard cellularDevices map with devMutex via snapshot/by-iface/set/remove
  helpers, mirroring the existing ethernet and wifi accessors
- Regenerate mocks with mockery v3 instead of hand-editing mock_Backend.go
- Reuse refreshCellularState() in SetCellularEnabled, DisconnectCellular,
  DisconnectCellularDevice, ActivateCellularConnection and device removal
- Wrap retryErr rather than the first error when the auto-config retry fails
- Use Theme.surfaceTextSecondary/primaryHover/primaryHoverLight in the
  cellular-off state instead of hand-rolled Qt.rgba values
- Drop the duplicate "No devices found" label so it can no longer render
  twice with no modem and no saved config

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kmf

kmf commented Aug 17, 2026

Copy link
Copy Markdown
Author

Still testing

@kmf

kmf commented Aug 17, 2026

Copy link
Copy Markdown
Author

Hardware testing done

Re-tested on the Fibocom L831-EAU-00 (Vodacom SA, cdc-wdm0) after the review fixes in e922cc9.

Build / unit

  • go build ./... clean
  • go test -race ./internal/server/network/... passes
  • go vet ./internal/server/network/... fails on manager_test.go:389 (assert.NotNil copies a syncmap.Map lock). This reproduces on a clean origin/master worktree too, so it is pre-existing and not from this PR — flagging in case you want it fixed separately.

Cellular against the real modem

Driven over the backend socket (API v31, NetworkManager backend), starting from a deleted GSM profile so the create-from-scratch path was exercised:

Operation Result
connect, no saved profile AddAndActivateConnection created + activated "Mobile broadband", IP 100.65.208.90
state after connect device activated, connection listed isActive, cellularConnectionUuid set, preference → cellular
disconnect disconnected, IP/uuid cleared, profile retained
connect, existing profile reconnected via ActivateConnection, IP 10.68.140.18
radio disable / enable unavailableactivated, IP 100.88.195.33
radio toggle back to unavailable

All four refreshCellularState() steps (devices, state, connections, primary connection) propagate correctly in every case, so the de-duplication in that refactor is verified end to end.

UI

Control Center cellular panel renders correctly in both states:

  • Radio off — muted Theme.surfaceTextSecondary icon and the "Enable Cellular" button in Theme.primaryHoverLight, consistent with the rest of the theme (this is the hand-rolled Qt.rgba fix).
  • Connected — single "Mobile broadband / Connected" row, active border, disconnect action, and no stray "No devices found" label.

Two things I could not prove on this hardware

  1. The auto-config retry branch never executed. The first AddAndActivateConnection succeeds with auto-config on this modem, so the retryErr wrap was only observed on an unrelated polkit-denied attempt (Not authorized to control networking. — my own fault for launching the backend from an ssh session rather than the graphical one). The wrap is correct by inspection, but a stack that genuinely rejects gsm.auto-config would be needed to exercise it for real.
  2. The exact duplicate-label scenario was not reproduced. It requires zero modems present; unbinding the USB device seemed more invasive than warranted. With a modem attached only the correct single empty state is reachable, so that specific double-render rests on code inspection.

@kmf
kmf requested a review from bbedward August 18, 2026 09:33
cellularToggling = false;

if (response.error) {
ToastService.showError(I18n.tr("Failed to enable cellular"), response.error);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is always "failed to enable cellular" even if its a failed to disable. I would just replace the string with "Failed to toggle cellular"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 29bc88d — the error toast is now Failed to toggle cellular.

Comment thread quickshell/Services/NetworkService.qml Outdated
}
}

function enableCellularRadio() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used so I'd just delete it, in addition to the function in DMSNetworkService

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 29bc88d — removed enableCellularRadio() from both NetworkService.qml and DMSNetworkService.qml. The backend network.cellular.enable IPC handler is still there.

types.push("cellular");
return types.length > 0 ? types : ["wifi"];
}
property int selectedTypeIndex: -1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it would be more robust to store the selected value instead of index

property string selectedType: ""
readonly property string currentConnectionType: {
    if (selectedType && connectionTypes.includes(selectedType))
        return selectedType;
    return connectionTypes[Math.max(0, currentPreferenceIndex)] || "wifi";
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 29bc88dselectedType is now a string, and currentConnectionType uses it when it is still in connectionTypes, otherwise it falls back to preference. If a NIC disappears while the panel is open the pane no longer remaps to a different medium via a stale index.

if (!selected)
return;
NetworkService.setNetworkPreference(index === 0 ? "ethernet" : "wifi");
selectedTypeIndex = index;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here instead of index:

currentIndex: Math.max(0, connectionTypes.indexOf(currentConnectionType))
onSelectionChanged: (index, selected) => {
    if (!selected)
        return;
    selectedType = connectionTypes[index] || "wifi";
    NetworkService.setNetworkPreference(selectedType);
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 29bc88dcurrentIndex is connectionTypes.indexOf(currentConnectionType), and onSelectionChanged writes selectedType from the chosen value.

- Toast "Failed to toggle cellular" instead of always "enable"
- Remove unused enableCellularRadio() wrappers
- Store Control Center network type as a value, not a list index
@kmf
kmf requested a review from bbedward August 19, 2026 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants