Fix getting Modified date into JSON when returning file list to WebUI#1606
Fix getting Modified date into JSON when returning file list to WebUI#1606michmela44 wants to merge 4 commits into
Conversation
|
I'm a little concerned about including information that is very likely to be wrong. Maybe it would be a good idea to try and set the clock with NTP so the dates on uploaded files have some chance of being correct. |
|
Yeah, I wasn't real sure about that. It seems in some cases my dates are correct, but sometimes they aren't. I'll dig into it and see what we can do. |
|
If you uploaded a file to the SD Card via WebUI, the date will be wrong because the ESP32 does not have a battery-backed clock. It might be possible to set the ESP32's RTC on every reboot via NTP or an interaction with WebUI. |
b0ad2f2 to
7eb297f
Compare
|
@MitchBradley I added NTP time sync It only runs if there is a successful connection in STA mode. I also added a $NTP/Enable setting that defaults to ON. Wasn't sure if this was required, but can remove it if you like. It tries pool.ntp.org first, then time.google.com, then falls back to attempting any discoverable NTP servers on the local network from the DHCP server It uses the SNTP library. My understanding is that this all happens on the LWIP context in it's own timer, so it shouldn't block anything in the main loop |
|
Nice |
|
I did some local tests to verify the setting worked, and that anything I uploaded, or wrote locally like preferences.json had the correct UTC time. I didn't bother with time zones or anything like that because I figured we'd handle that at the display, and that's easier if we know its always in UTC. Build shows ~40 bytes increase in RAM from before changes |
In my humble opinion, the order should be local first, then pool.ntp.org, and then maybe time.google.com |
That's a fair point. I can see the reasoning behind prioritizing local sources, and it was something I considered. Let me explain my thinking: The majority of FluidNC users are in home/hobby environments where a local NTP server isn't available. While consumer routers can run NTP servers, they rarely do by default, and most ISPs don't provide NTP services to their customers. My concern with putting local first is the timeout penalty. If there's no local NTP server (which will be true for most users), they'll wait 6-10 seconds for the request to fail before falling back to a public server. I wanted to optimize for the common case where external servers are the only option. That said, I included the local option specifically for users with homelabs or more sophisticated network setups who do have local NTP available. My opinion is that if you both have the expertise to set up and run a local NTP server, and also want to guarantee your local server gets used over public NTP servers, then you can probably just as easily block the traffic from FluidNC to the public servers and force only yours to work. However, if @MitchBradley thinks the timeout delay is acceptable for the benefit of checking local first, I'm open to looking into it and seeing if we can prioritize local. There's also the possibility that options can be added to let you specify the servers to use, or change the priority order, etc., but I didn't think it was worth the trouble at this stage to go that far. |
|
Thank you for your detailed response, I really appreciate it. Please do not let my comment complicate the addition of this feature by requiring extra configurables. As you said, this can come later. |
|
So many things to go wrong. |
There was a problem hiding this comment.
Pull request overview
This PR targets the WebUI filesystem listing response by populating the datetime field with each file’s last-modified timestamp (ISO 8601 UTC), and adds WiFi-triggered SNTP initialization so the device clock can be correct when producing timestamps.
Changes:
- Populate
datetimein the WebUI file list JSON usingdirectory_entry::last_write_time()formatted asYYYY-MM-DDTHH:MM:SSZ. - Add SNTP start/stop logic tied to WiFi connection lifecycle, plus a Web setting to enable/disable NTP sync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| FluidNC/src/WebUI/WifiConfig.cpp | Adds SNTP (NTP) initialization/shutdown on WiFi connect/disconnect, plus an NTP/Enable setting. |
| FluidNC/src/WebUI/WebUIServer.cpp | Implements last-modified timestamp extraction/formatting for file list JSON datetime. |
| char buf[32]; | ||
| if (std::strftime(buf, sizeof(buf), "%Y-%m-%dT%H:%M:%SZ", std::gmtime(&cftime))) { |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
Fixes getting the datetime set into the result json for file lists in the WebUI.
Currently, it's always blank.
This change makes it return
{"files":[ {"name":"config.yaml", "shortname":"config.yaml", "size":"7372", "datetime":"1970-01-01T00:29:31Z" }, {"name":"favicon.ico", "shortname":"favicon.ico", "size":"1150", "datetime":"2025-10-03T18:56:38Z" }, {"name":"index.html.gz", "shortname":"index.html.gz", "size":"106168", "datetime":"1970-01-01T00:06:34Z" } ], "path":"", "total":"192.00 KB", "used":"124.00 KB", "occupation":"64", "status":"Ok" }