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

Add firmware upgrade notification to status page #7597

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aparcar
Copy link
Member

@aparcar aparcar commented Jan 30, 2025

This is likely not cleanly implemented, I'm unfamiliar with the LuCI tricks and JavaScript in general.

The core idea is to show a notification whenever a new firmware is available.

image

Right now it's just a notification, this could be extended to include the luci-app-attendedsysupgrade functionality or a slimmed down version.

@jow- please have a look

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: (architecture, openwrt version, browser) ✅
  • ( Preferred ) Mention: @ the original code author for feedback
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • Description: (describe the changes proposed in this PR)

This allows each include to perform an action a single time but don't
run it on each poll. The function receives all data loaded via the
`load` function.

Signed-off-by: Paul Spooren <mail@aparcar.org>
If the UCI luci core section contain the option `check_firmware_version`
then a load of the status page checks if the download server offers a
new firmware compatible with the currently running device.

Signed-off-by: Paul Spooren <mail@aparcar.org>
&& boardinfo.release.version > data.stable_version
&& data.upcoming_version > boardinfo.release.version
&& checkDeviceAvailable(boardinfo, data.upcoming_version)) {
showUpgradeModal("upcomming", boardinfo.release.version, data.upcoming_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

upcoming (or just new?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe "release candidate"? since I think that's the only thing ever to populate the upcoming_release field in .versions.json...

oneshot: function(data) {
var boardinfo = data[0];
var check_upgrades = uci.get_first('luci', 'core', 'check_firmware_version') || false;
console.log(check_upgrades)
Copy link
Contributor

Choose a reason for hiding this comment

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

drop

}
})
.catch(error => {
console.error('Failed to fetch profile information:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding more context to the error. A user who doesn't know about this feature might be confused by this err string, even though it's a console message.

@systemcrash
Copy link
Contributor

Consider using ui.addNotification() with perhaps a 60 second timeout, instead of ui.showModal(). Having a modal that a user must interact with to reach the interface causes friction (e.g. when a user logs in and perhaps needs to take care of something else more pressing).

// ui.addNotification(a, message, timeout, severity)
ui.addNotification(null, E('p', _('A new firmware is available.')), 60000, 'info');

]);
},

oneshot: function(data) {
var boardinfo = data[0];
var check_upgrades = uci.get_first('luci', 'core', 'check_firmware_version') || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but I think that should be 'main' and not 'core', assuming the js versions work like the cli tool:

$ uci show luci.core
uci: Entry not found

$ uci show luci.main
luci.main=core
luci.main.lang='auto'
luci.main.mediaurlbase='/luci-static/openwrt2020'
luci.main.resourcebase='/luci-static/resources'
luci.main.ubuspath='/ubus/'

@hnyman
Copy link
Contributor

hnyman commented Jan 31, 2025

Is this forced to all users? Also for those who regularly build their own builds every few days...

I am wondering if this should either

  • be non-default, a small LuCI package that is included in the release branch buildbot images but would not get be default into private builds, or
  • be called via a menu item (so that it is not in the front page).

In many OEM firmwares that I have seen, the upgrade check/notice is in a system admin menu, not on the front page.

(And hopefully the data is not polled by every 5 seconds)

@systemcrash
Copy link
Contributor

(And hopefully the data is not polled by every 5 seconds)

At most once a week is optimistic, once a month preferred (cronjob trigger?). But that's probably why the function is a oneshot. I suppose it could have a counter, and disable itself after a few tries, only to be activated after the next upgrade / reboot.

@aparcar
Copy link
Member Author

aparcar commented Jan 31, 2025

It runs every time you open the status page and does nothing in the background, why should it anyway.

It's not enabled by default but could be if users want that. A modal could open once and ask the user if they want to automatically check for upgrades when opening the web interface.

Additionally we could add a button somewhere to manually switch.

@efahl
Copy link
Contributor

efahl commented Jan 31, 2025

Additionally we could add a button somewhere to manually switch.

Putting a "Don't show me again" button right on the modal dialog would probably be pretty easy, it could just do uci.set('luci', 'main', 'check_firmware_version', 0) like is done at https://github.com/openwrt/luci/blob/master/modules/luci-mod-system/htdocs/luci-static/resources/view/system/system.js#L270

@hnyman
Copy link
Contributor

hnyman commented Jan 31, 2025

Putting a "Don't show me again" button right on the modal dialog would probably be pretty easy,

I feel that any automatic call-to-home should be opt-in, not something to be opted out. By default, the OpenWrt routers are currently not contacting OpenWrt servers before the user selects some action like opkg/APK that triggers the contact.

That "no automatic call-home" was the OpenWrt stance a few years ago, when there was discussion about collecting usage stats, device counts etc.

@aparcar
Copy link
Member Author

aparcar commented Jan 31, 2025

This is out of the question, it'd always be an opt in feature.

I'll see how to implement that and update this PR accordingly

@efahl
Copy link
Contributor

efahl commented Jan 31, 2025

It's already opt-in. The value for luci.main.check_firmware_version is undefined, so it results in false. You have to make an effort to explicitly add it to /etc/config/luci to turn it on...

@richb-hanover
Copy link

A couple thoughts:

It's already opt-in. The value for luci.main.check_firmware_version is undefined...

We could use this to our advantage to handle the "startup case". When that value is undefined, it could be a trigger for LuCI to pop up the "Opt in/Opt out" window. We could treat the Yes and No answers in the obvious way, and treat a click on "ask me later" or simply dismissing the window by leaving it undefined. (We would probably implement similar behavior for the CLI as well...)

Once we get permission, the question is "when does the router check?" I am reluctant to queue up any kind of cron job because it creates a permanent load on our servers.

My inclination would be to (both) request opt-in/opt-out and check for updates when there's interesting activity in LuCI or the command line, perhaps at login. That way we can be certain that there's a human present who can view and act on the "New Version" notice.

@efahl
Copy link
Contributor

efahl commented Feb 15, 2025

perhaps at login.

Yeah, that's the current behavior. You log in, it pops up the notice. I've had this PR running on my main router (23.05.5) for a couple weeks and it simply shows the dialog every morning when I log in for status. It's pretty non-intrusive...

@richb-hanover
Copy link

More thoughts:

  • What about Snapshot versions? I think they should always be ignored. If you're running a snapshot, you are almost certainly aware that there's a new snapshot available now (or after you wake up tomorrow).

  • What about Release Candidates? I think they should always be checked. If you're running a RC, you likely would like to know about a new one.

  • The "version update check request" might contain the current version. I realize that this gets close to (or goes over) the line about not collecting user data. (Could we stand another flag to say, Don't supply my current version in the request?)

@efahl
Copy link
Contributor

efahl commented Feb 15, 2025

Currently snapshots are ignored by this, you need to go to ASU and do a "check for update" to get the check with those.

RCs are included as part of the latest list in https://downloads.openwrt.org/.versions.json, so are treated the same as a regular upgrade.

The download URL includes both the current version and target/subtarget of the device, which is the same as when doing ASU's "check for newer", so if you were already doing that, this won't leak any more information than you already did.

@aparcar
Copy link
Member Author

aparcar commented Feb 17, 2025

To move things forward I'd take the logic from this PR and add it to luci-app-attendedsysupgrade. Instead of messing with the system stuff we should use a status include as shown here

@systemcrash
Copy link
Contributor

Given that we don't live in a perfect world, a link to the current release notes would be my recommendation (firmware selector does not have this prominently displayed anywhere). Rare, but sysupgrades can result in a brick.

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.

5 participants