wui: Allow overriding the NTP server from the settings ini - #5417
wui: Allow overriding the NTP server from the settings ini#5417henrikottesorensen wants to merge 4 commits into
Conversation
|
Please do test it correctly on hardware, some bits (including the init) do not seem right. Just running it on the simulator isn't enough, test it on real hardware. Claude is known for hallucinating a lot of stuff, at least use a second AI engine to verify Claude's results. I did my PR based on fixing gh issues, that's why I added other parts as well in order to prevent having to rewrite the code again every time. |
62f138d to
857ac99
Compare
Sigh... I guess I'll have to schedule an appendectomy surgery. Asked 3 models for a blind audit. We found some initialisation concurrency problems, and have added test coverage. Update: Surgery was a success, a master build with commits from this PR. NTP traffic flows to the configured NTP server, and with |
7ef1911 to
3d2ad1c
Compare
The SNTP client is hardcoded to prusa3d.pool.ntp.org and there is no way to point it elsewhere, so a printer on a network without a route to the internet can never obtain a time. On printers without a backup battery this is every boot, which leaves timestamps at 1970 and breaks TLS certificate validation for Prusa Connect deployments using hostname/custom_cert from this same ini. Add an ntp key to the [network] section of prusa_printer_settings.ini, stored in the config store beside the hostname and applied after sntp_init(), which unconditionally resets server 0 to the compiled-in default. The setting is empty by default and loading an empty value reverts to the default, so behaviour is unchanged for anyone who does not set it. BFW-6184 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run the real sntp.c and sntp_client.c on the host, with wui_get_ntp_server() and netdev_get_status() stubbed, and check which server name ends up registered as server 0: * The compiled-in default when no override is configured. * The configured override, which only holds when it is applied after sntp_init() -- sntp_init() unconditionally resets server 0, so a wrongly ordered init would fail this case. * A config change is not picked up while the client is running, only after it observes the interface go down and up again. * Clearing the override reverts to the default on such a cycle. The LwIP file list and the linker stubs follow the approach of the neighbouring nhttp tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The tests stubbed HAS_ESP() to 0, which compiled the wifi half of sntp_client_step() out entirely. Both settings ship: iX and XL_DEV_KIT build without it, while MK4, MK3.5, XL, MINI and the Core Ones build with it. Build the same sources twice, once per stub directory, rather than covering one and assuming the other. The netdev stub now answers per interface. It could not before -- it ignored the id and returned one status for both -- so turning HAS_ESP() on alone would have ORed a value with itself and tested nothing new. Two cases only the wifi build can express: * An ethernet down/up is invisible while wifi stays associated, because sntp_client_step() ORs the two interfaces. There is no poll interleaving that could observe the bounce, so a changed server is not applied at all while wifi holds the client up. The case documents that limitation as it currently stands; it is a stronger statement than the ethernet-only case, where the poll merely usually misses the transition -- here it cannot see it. * Wifi hardware absent, which is not a separate build: the critical-infrastructure editions of the XL and the Core One run this firmware with the wifi circuitry physically removed and the wired port kept, so NETDEV_ESP_ID simply never comes up. Those printers are also the best case for the setting itself, being wired machines on deliberately offline networks where the compiled-in pool address cannot resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A settings reload stored a new NTP server but did not use it. sntp_client_step() re-initialized the client only on an observed netif down->up transition, and reconfigure() bounces the interfaces within a single pass of the network loop -- the same pass that polls SNTP a few lines later -- so the poller never observes the down state. Found by review, then reproduced: on a printer with a static IP, loading an ini with an ntp key never sent a single packet to the configured server, and the printer went on using the compiled-in default until the next reboot. With DHCP the lease negotiation usually keeps the interface down long enough for a later poll to catch it, which is why this hid: it bites exactly the static configurations that offline networks, the ones this setting exists for, tend to use. With wifi associated the bounce is structurally invisible, as the previous commit documents. And clearing the override was worse than not applying it: lwIP's sntp_setservername() stores the pointer it is given, and it pointed into the config store's buffer, which the reload had just emptied in place, leaving the client resolving an empty name. An explicit reset-on-reconfigure notification was tried first and kept springing smaller leaks of the same shape: a reset landing while every interface is down stranded a live pcb and retry timer, and the re-init path violated sntp_setoperatingmode()'s not-running contract. Transition-detection needs to see every event, and this loop cannot promise that. So reconcile instead: each pass compares what should be true -- some interface up; the server the config store currently wants -- with what the lwIP client is doing, and stops or restarts it to match. Transitions stop mattering, so the unobservable ones stop being bugs, and reconfigure() needs no notification at all. The details that carry the design: * The client registers a private copy of the name, not the config store's buffer, so nothing lwIP reads is rewritten behind its back. The copy itself is taken under the tcpip core lock, because lwIP still holds the pointer from the previous start and may be reading it in an in-flight request on the tcpip thread. * The restart stops the client first, and before sntp_setoperatingmode(), which asserts that the client is not running. The stop is also what makes the restart real: sntp_init() short-circuits on an existing control block and would re-apply the server name without ever scheduling a request. Neither mistake is fatal when made -- a failed lwIP assert only logs on this firmware -- which is why the tests now count assert calls instead of ignoring them: every case fails if any lwIP contract is violated, visibly or not. * The override is applied after sntp_init(), which unconditionally resets server 0 to the compiled-in default, as before. The tests that asserted the old transition-only behaviour now assert the reconciliation: a config change is applied on the next pass with no interface cycle at all, including under a wifi association that hides an ethernet bounce completely, and the client is verifiably stopped -- through sntp_enabled(), lwIP's own pcb state, not the latch -- whenever no interface is up, config changes included. Mutation checked: reverting the reconciliation to transition-detection, disabling the stop-when-down, and re-ordering the restart each fail exactly the cases that assert that behaviour, in both HAS_ESP configurations, and leave the rest passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3d2ad1c to
7812ee6
Compare
Refers to #3261. A deliberately minimal subset of what #3679 attempted.
Problem
The SNTP client is hardcoded to
prusa3d.pool.ntp.org(lib/WUI/sntp/sntp_opts.h) and nothing can override it:sntp_client_initnever callssntp_setservername, and DHCP-supplied NTP is compiled out (LWIP_DHCP_GET_NTP_SRVdefaults to 0 and is not raised anywhere).On a network with no route to the internet, that leaves a printer with no way at all to learn the time. Not a degraded way — none:
lib/WUI/wui_api.cpp), so if SNTP cannot reach a server the clock is never set by anything;MI_TIMEZONEandMI_TIMEZONE_MIN— an offset applied to an SNTP-sourced time — andMI_TIME_NOW, which only displays. There is no manual date entry, so the operator cannot supply what the network won't;This is the ordinary condition for a printer on a segregated or air-gapped network — a school, a lab, an office VLAN with no outbound access, a workshop behind a firewall that permits nothing outbound by default. Several people in #3261 describe exactly that, including the case where the operator has no authority to change DNS or firewall rules even if a workaround existed. The commonly-suggested workaround, overriding
prusa3d.pool.ntp.orgin local DNS, requires precisely the access those users do not have — and breaks under DNSSEC or DoH.It is also the intended condition for the critical-infrastructure editions, which ship with the Wi-Fi circuitry physically removed and the wired port kept: a machine deliberately placed on a network with no internet route, where the compiled-in pool address can never resolve.
And on a printer with no backup battery this is every single boot, not a one-off: it comes up at the epoch, and stays there.
The visible symptoms are the ones reported in #3261: files uploaded over PrusaLink dated 1970 while SD-card files are correct; print history and timestamps that cannot be trusted; and — for anyone using this same ini's
hostname/custom_certto point a printer at their own server — TLS certificate validation that cannot succeed, because certificate validity is checked against a clock that was never set.Worth stating plainly: this is a problem Prusa Connect users do not have, because reaching Connect means reaching the internet by definition. That is presumably why it has sat since 2023. But the same ini already carries
hostnameandcustom_cert, which exist so a printer can be pointed at a server that is not Prusa's — so the firmware already supports the deployment that this gap breaks.Change
An
ntpkey in the[network]section ofprusa_printer_settings.ini:Accepts a hostname or an IP literal (
SNTP_SERVER_DNSis already 1, and lwIP resolves address literals without a DNS server). A.localname does not work and the ini says so:LWIP_MDNS_RESPONDERis enabled butLWIP_DNS_SUPPORT_MDNS_QUERIESis not, so a printer answers mDNS queries about itself without being able to resolve anyone else's name. On a network with no DNS server at all, an IP address is the only form that works.Stored in the config store beside
hostname, and plumbed throughnetif_config_twith anETHVARbit exactly ashostnameis — deliberately, becausenetdev_load_esp_credentials_eeprom()parses the same file into a throwaway config to keep only wifi credentials, and a store-direct write would make loading wifi credentials silently persist an NTP server too. The mask pattern is immune to that by construction.Empty by default, so behaviour is unchanged for anyone who does not set it, and loading an empty value reverts to the compiled-in default.
Subtle part 1: the override must be applied after
sntp_init()sntp_init()itself callssntp_setservername(0, SNTP_SERVER_ADDRESS)unconditionally, so a name set before it is silently overwritten and the setting appears to do nothing. The comment is in the source because this is easy to get wrong and invisible when wrong.Subtle part 2: a settings reload does not re-initialise SNTP by itself
This one was a defect in the first version of this PR, found after review and re-audits. Storing the new server was not enough to use it.
sntp_client_step()re-initialises the client only on a netif down→up transition, andreconfigure()brings the interfaces down and back up within a single pass of the network loop — the same pass that callssntp_client_step()a few lines later (wui.cpp). The poller therefore never observes the down state and thesntp_runninglatch stays set. On printers with Wi-Fi it is worse still:sntp_client_step()ORs the two interfaces, so while Wi-Fi is associated an Ethernet bounce is not merely likely to be missed, it is unobservable.With DHCP the lease negotiation usually keeps the interface down long enough for a later poll to catch it, which is why this hid — it bites exactly the static configurations that offline networks tend to use.
Clearing the override was worse than not applying it.
sntp_setservername()stores the pointer without copying, and it points into the config store's own buffer, which the reload has just emptied in place — so without a re-init the client was left resolving an empty name and time sync stopped altogether instead of reverting to the default as documented.Fixed by reconciling rather than edge-detecting. The first attempt notified the client from
reconfigure(), and that turned out to be the wrong shape: acting on observed transitions means every transition the poller cannot observe needs its own side channel, and review found a third hole of the same kind before the pattern was clear.sntp_client_step()now compares what should be true — some interface up, and the server the config store currently wants — against what the lwIP client is doing, and stops or restarts it to match. Transitions stop mattering, so the unobservable ones stop being bugs: a reload is picked up within one pass whatever the interfaces did in between, and the client is never left running with everything down. The notification and its call site are deleted.The comparison is sound only because the client keeps a private copy of the applied name and registers that with lwIP.
sntp_setservername()stores the pointer it is given, and the config store rewrites its buffer in place on a reload — so registering that buffer both handed lwIP a name that could change under an in-flight query and made the comparison compare the buffer with itself. The copy closes both.sntp_client_start()still stops before initialising, becausesntp_init()short-circuits when the UDP control block already exists andsntp_setoperatingmode()asserts the client is not running.Verified
All of the below is emulation (MINI404,
prusa-mini, firmware built from this branch), capturing guest Ethernet frames with-object filter-dumpand decoding with tshark.The setting reaches the wire
prusa3d.pool.ntp.org.example10.0.2.2The second row is the control: on the patched firmware with nothing configured, the compiled-in default is still used, unchanged. The third and fourth rows differ from it only in the stored value, and the compiled-in default is then queried zero times — which is what demonstrates the ordering above is correct rather than merely plausible. The third row's name is deliberately in the reserved
.exampleTLD, so it cannot resolve; the ten queries are lwIP's own SNTP retry schedule, and what matters is which name was asked for. The last row is a real NTP client packet on the wire addressed to the configured server.The ini file is what sets it
Three further runs, each booting the same firmware with a freshly generated EEPROM containing no
NTP Serveritem, differing only in thentp=line on the emulated USB stick, and each loading the file through the printer's own Settings → System → Load settings from file:[network] ntp=prusa3d.pool.ntp.orgprusa3d.pool.ntp.orgagainntp.example.lanprusa3d.pool.ntp.orgntp.example.lan+ retries, and zero furtherprusa3d.pool.ntp.org192.168.13.1prusa3d.pool.ntp.orgThe before-column is what makes the after-column evidence: all three query the compiled-in default first, proving the config store started empty, and the control queries it again after loading, proving the load itself is not what moves the target. Only the ini differs.
The third run is the point of the whole change: an IP literal never touches DNS, so that packet cannot be a resolver accident, and the printer took its clock from the LAN router — the air-gapped case above, working.
Incidentally corroborated by a key travelling the same code path: the same ini sets
hostname=ntplab, and the printer's mDNS announcements change fromprusa-mini.localtontplab.localin all three runs.hostnamegoes through the sameini_handler_func→var_mask→save_net_paramssequence, so it witnesses the parse independently of anything SNTP does — and shows the control run did parse the file, rather than simply doing nothing.A reload takes effect without a reboot
The runs above use DHCP. Repeated with a static IP, which is what exposed subtle part 2:
ntp=192.168.13.1192.0.2.1, thenntp=emptied192.0.2.1stop and nothing replaces them — time sync silently deadprusa3d.pool.ntp.org, NTP answered — reverts to the default as documentedIn the first row the ini demonstrably did load in the broken build — the hostname was applied and mDNS re-announced in that same run — which isolates the failure to SNTP re-initialisation rather than to parsing or storage.
Unit tests
tests/unit/lib/WUI/sntpruns the realsntp.candsntp_client.con the host withwui_get_ntp_server()andnetdev_get_status()stubbed, and checks which server ends up registered as server 0. Built twice, because both settings ship: without Wi-Fi (iX, XL_DEV_KIT) and with it (MK4, MK3.5, XL, MINI, the Core Ones).Covered: the default when nothing is configured; the override, which only holds when applied after
sntp_init(); that a config change is applied on the next pass with no interface cycle at all, including under a Wi-Fi association that hides an Ethernet bounce completely; that clearing it reverts to the default; that absent Wi-Fi hardware behaves like Ethernet only; and that the client is verifiably stopped whenever no interface is up — throughsntp_enabled(), lwIP's own control-block state rather than the driver's own flag. The stub forlwip_platform_assert()counts invocations and every case asserts none fired, which is what catches lwIP contract violations — on the firmware a failed assert only logs and execution continues, so nothing observable would otherwise go wrong.On real hardware
An MK3.5 (xBuddy) running a build of this branch, wired LAN, router at
192.168.13.1also serving NTP. Captured withtcpdumpon the router.Configured server, from a cold boot —
ntp=192.168.13.1already in the config store:No DNS lookup precedes it — an IP literal never reaches the resolver — and
prusa3d.pool.ntp.orgis not queried anywhere in the capture.Cleared again, without a reboot — loading an ini whose
ntp=is empty:2.1 s from the reconfigure to the request, with no interface cycle arranged by hand and no power cycle — the reconciliation noticing a config change on its own, which is what it exists for. This is precisely the case the earlier notification-based version got wrong.
The parse is corroborated as in emulation: the same ini sets
hostname, and the printer's mDNS identity changed to match.One observation worth recording because it is easy to misread as a fault: there were 20–30 s of complete silence between the settings load and the mDNS burst. That is the interface being down across the bounce — link renegotiation, and most likely spanning-tree forwarding delay on the switch port — during which the client is deliberately stopped because no interface is up. It is not specific to this change; any ini load touching
[network]callsnotify_reconfigure()and has always done this. It is also invisible in emulation, where there is no physical link.What this is not: the firmware was self-built from this branch rather than one of your signed releases, and the test LAN has a working route to the internet — so the offline deployment the feature exists for is still inferred from the mechanism rather than demonstrated end to end.
Also built clean for
mini_release_boot,mk3.5_release_bootandmk4_release_noboot. The change also cherry-picks ontov6.5.7with a single trivial conflict (6.5.7 sources two sizes fromold_eeprom::where master inlines literals) and builds clean formk3.5_release_bootthere —sntp_client.c,sntp_client.h,wui_api.cppandnetif_settings.hare byte-identical between the two refs, so backporting to the shipping line needs no adaptation.Cost, measured on
mini_release_bootagainst the branch point (e96ce2b9) rather than estimated:firmware.binsize.data(objdump -h).bss(objdump -h)So 56 bytes of RAM in total: 24 for the config store's in-RAM mirror growing for the new 31-character item, and 32 for the client's private copy of the applied server name.
arm-none-eabi-sizefolds.dataintotexthere and counts._user_heap_stackinbss, hence the section-level figures above. Nothing is written to EEPROM unless the key is actually set.Scope of the verification, stated plainly: the emulation covers the ini-parsing and config-store legs with controls; the hardware run covers both directions of the setting on a real MK3.5, from a self-built image of this branch. Not covered: an official signed build, any printer other than the MK3.5, and the genuinely offline network the feature targets.
Scope
Deliberately minimal, and deliberately smaller than #3679, which has been open since January 2024:
dns4is the precedent that an ini-only network setting is an accepted shape.sntp_opts.hor any lwIP option;SNTP_MAX_SERVERSstays 1 and no fallback-slot scheme is introduced.LWIP_DHCP_GET_NTP_SRVplussntp_servermode_dhcp()would cover the other half of [BFW-6184] [ENHANCEMENT] Missing option for custom NTP or NTP via DHCP #3261's title and is genuinely separable — but it raises questions this change does not have to answer (which interface's DHCP wins when one is static and one is not; whether a DHCP-supplied server should be allowed to replace a configured one; whether it should default on at all). Happy to follow up if wanted.Credit to @bkerler, whose #3679 covers a superset of this; this is the subset that seemed most likely to be mergeable on its own.
Written with Claude Code; the commits carry
Co-Authored-Bytrailers so the attribution survives the cherry-pick into your internal branches.BFW-6184