network/time: Add option to set ntp ip address or to set ntp via dhcp #3679

bkerler · 2024-01-13T15:02:16Z

Refers to issues #3523 and partly #3671.

This commit adds two new features in order to set the network time automatically :

Default: Use the hardcoded ntp ip (as it was before).
Setting the ntp ip address via dhcp server instead, selectable in the GUI via Settings -> Network.
Add an option to set the ntp ip address manually.
Store this as "prusa_printer_settings.ini" and upload to usb_drive:

[network]
ntp4=195.113.144.238

or

[network]
ntp4=pool.ntp.org

Then load via "Settings -> System -> Load Settings" in the GUI.

The PR also adds a label in "Settings -> Network -> NTP Address" to show the current NTP address.

The static ntp address "pool.ntp.org" is being stored in the eeprom.
This also enables the option in the future for a menu entry in the gui itself in order to set the ntp address manually using scroll wheel (which isn't yet implemented).

bkerler · 2024-01-13T16:05:49Z

Option 3 (via gcode) will be replaced with configuration file as gcode doesn't feel right for setting up the static ntp ip. Thanks @vorner for the hint.

vorner

I'm a bit worried about the setting of the NTP to gateway, otherwise there are some nitpicks.

lib/WUI/sntp/sntp_client.c

lib/WUI/wui_api.cpp

src/gui/MItem_lan.cpp

thess · 2024-02-08T14:53:06Z

I think this is a good approach to resolving most of the issues with network time setting. Having a fixed IP for NTP is useful in a number of situations but as a global solution it falls short. I found that enabling DNS and settting the default server to pool.ntp.org gives the best result for the majority of your installations - worldwide. See #3730

bkerler · 2024-02-08T23:21:30Z

@thess I'm going to add your commit into mine, if that's ok ?

bkerler · 2024-02-08T23:59:39Z

Integrated PR #3730 (Pool as default & DNS support)

thess · 2024-02-09T16:33:06Z

Looking this over, I don't think it works the way you are expecting. I see a number of problems with the configuration and code flow. Notably:

No need to use LWIP_IPV4 conditional in sntp_client.c (If no LWIP then no need for SNTP, DNS or any network config)
SNTP_MAX_SERVERS is 1 (set by LWIP_DHCP_MAX_NTP_SERVERS) -- therefore setting indexes other than 0 do nothing.
lwip_gethostbyname() can take a string argument with an IP address and return correct info. No need to differentiate.
Need to store NTP server/pool name or NTP address as a string in config_store. It should only be an address if it is a fixed local/private server. Public servers addresses WILL change - use DNS when possible. (Default: pool.ntp.org)
LWIP_DHCP_GET_NTP_SRV is not set, so using DHCP for NTP server is not functional.

I also object to putting a hard-coded IP address into the firmware. It will bite you someday.

I can re-work my PR to encompass your proposed functionality and re-submit it if you would like - your choice.

bkerler · 2024-02-10T18:16:13Z

I'm using the LWIP_IPV4 define intentionally as currently, IPV6 support is prepared.
Thanks for pointing out the SNTP_MAX_SERVERS, will fix that
Ok, will then use lwip_gethostbyname for ip as well
Storing the ntp server/pool name is a problem, as the space is limited ... need to discuss that with the devs
LWIP_DHCP_GET_NTP_SRV .... this one isn't needed as SNTP_GET_SERVERS_FROM_DHCP is already defined in lwipopts.h as 1

I will add the fixes and test them on the hardware and simulator :)

bkerler · 2024-02-10T19:53:22Z

The commit does now reflect the needed changes.

thess · 2024-02-10T23:43:06Z

Storing the ntp server/pool name is a problem, as the space is limited ... need to discuss that with the devs

Network configuration can get messy. You can support 95% of situations with well-thought-out defaults. The last 5% costs time and space - no avoiding it. You could probably get by with 48-64 chars instead of 128!
Hope you have space for IPv6.

LWIP_DHCP_GET_NTP_SRV .... this one isn't needed as SNTP_GET_SERVERS_FROM_DHCP is already defined in lwipopts.h as 1

Without LWIP_DHCP_GET_NTP_SRV set for lwIP build, there will be no NTP server (DHCP type 042) found and therefore no callback. Routers in general don't supply this unless specifically configured for something like ActiveDirectory or a private time server.

bkerler · 2024-02-11T16:23:23Z

Network configuration can get messy. You can support 95% of situations with well-thought-out defaults. The last 5% costs time and space - no avoiding it. You could probably get by with 48-64 chars instead of 128! Hope you have space for IPv6.

Indeed, the char length is something I need to discuss with the Prusa Devs .. including if they will allow it at all. IPv6 is already tested and it works with only minor changes needed. However it does need a clear separation of the web services like PrusaConnect ... but that's on my current toDo list.

LWIP_DHCP_GET_NTP_SRV .... this one isn't needed as SNTP_GET_SERVERS_FROM_DHCP is already defined in lwipopts.h as 1

Without LWIP_DHCP_GET_NTP_SRV set for lwIP build, there will be no NTP server (DHCP type 042) found and therefore no callback. Routers in general don't supply this unless specifically configured for something like ActiveDirectory or a private time server.

It should be defined as 1 using the LWIP_DHCP_GET_NTP_SRV but in my tests it didn't work. I will try to trace back if it is set, so thanks again for your hint. The devs said that it might not be added at all, but I wanted to have it included at least (if it does work), as it was requested to be used in special environments where custom routers with own time servers are needed (like schools).

bkerler · 2024-02-12T10:48:47Z

DNS name length for the eeprom has now been limited to 60 chars as 128 chars was too long.

vorner

I still feel this must have some weird corner-cases in various situations.

Notice that both interfaces are brought up if possible, the setting in the menu is only which one is the default. And in case one of them is through DHCP, the other is through static IP, will it do DHCP based one, or static one? Will it try to initialize the ntp service twice?

vorner · 2024-02-14T16:20:21Z

include/buddy/lwipopts.h

+#define LWIP_ETHERNET                1
+#define LWIP_DNS_SECURE              7
+#define DNS_MAX_NAME_LENGTH          128
+#define DNS_MAX_NAME_LENGTH_EEPROM   60


Is this the right place for the constant? This thing is mostly for configuring LwIP itself.

normally it's DNS_MAX_NAME_LENGTH as already defined in the LWIP stack. I tried to use it in the constants.hpp but that isn't used by the lwip stack on init (and cannot be imported).

another option (maybe better) would be to use DNS_MAX_NAME_LENGTH-68 instead.

Used now DNS_MAX_NAME_LENGTH-68.

vorner · 2024-02-14T16:25:00Z

lib/WUI/sntp/sntp_client.c

+    #else
+    sntp_setservername(1, ntp_address);
+    #endif
+#endif /* LWIP_IPV4 */


This looks… weird. Both the LWIP_IPV4 thing, but more so the SNTP_SERVER_DNS thing… so in that case we ignore the provided address and use something hardcoded? And why there's no such thing in the static init thing?

That's the backup servername in case the first entry 0 fails. In this case it falls back to the ntp_address stored in the configuration store (it's not hardcoded). the LWIP_IPV4 define is a preparation for the ipV4+ipV6 combined stack, but can be added later as well of course.

I removed the LWIP_IPV4 define for now.

vorner · 2024-02-14T16:26:42Z

lib/WUI/sntp/sntp_opts.h

@@ -78,7 +78,7 @@
 * \#define SNTP_SERVER_ADDRESS "pool.ntp.org"
 */
 #if !defined SNTP_SERVER_DNS || defined __DOXYGEN__
-    #define SNTP_SERVER_DNS 0
+    #define SNTP_SERVER_DNS 1


These options probably should go to include/buddy/lwipopts.h (see the top of this file, it's included from there and this is just creating a default).

Yes, I will have a look.

Changed that.

vorner · 2024-02-14T16:27:23Z

lib/WUI/sntp/sntp_client.h

-void sntp_client_init(void);
-void sntp_client_step(void);
+void sntp_client_static_init(const char *ntp_address);
+void sntp_client_dhcp_init(const char *ntp_address);


These inits probably aren't called from the outside, so maybe they should be static / anonymous namespace and not exported.

sntp_client_static_init is used in wui.cpp, thus cannot be static. But I made sntp_client_dhcp_init static.

vorner · 2024-02-14T16:30:24Z

lib/WUI/wui_api.cpp

@@ -202,6 +208,7 @@ void load_net_params(ETH_config_t *ethconfig, ap_entry_t *ap, uint32_t netdev_id
        ethconfig->dns2_ip4.addr = config_store().wifi_ip4_dns2.get();
        ethconfig->lan.msk_ip4.addr = config_store().wifi_ip4_mask.get();
        ethconfig->lan.gw_ip4.addr = config_store().wifi_ip4_gateway.get();
+        strlcpy(ethconfig->ntp, config_store().lan_ntp_addr.get_c_str(), DNS_MAX_NAME_LENGTH_EEPROM + 1);


This looks weird, we are in the wifi section…

This is used in the prusa_settings.ini, so should be fine I think

I removed the lan_ prefix, as ntp is universal and not limited to lan or wifi

vorner · 2024-02-14T16:30:59Z

lib/WUI/wui_api.h

@@ -48,6 +48,7 @@ typedef enum {
    ETHVAR_DNS1_IP4, // ip_addr_t, dns1_ip4
    ETHVAR_DNS2_IP4, // ip_addr_t, dns2_ip4
    ETHVAR_MAC_ADDRESS, // is not included in ethconfig (used in stringifying for screen)
+    ETHVAR_NTP_ADDRESS, // char[256+1], hostname or ip


256 seems like a lot.

I need to update the comment as it is now 60+1 in fact.

Updated the comment

vorner · 2024-02-14T16:34:55Z

src/persistent_stores/store_instances/config_store/store_definition.hpp

    StoreItem<std::array<char, lan_hostname_max_len + 1>, defaults::net_hostname, journal::hash("LAN Hostname")> lan_hostname;

    StoreItem<int8_t, defaults::lan_timezone, journal::hash("LAN Timezone")> timezone; // hour difference from UTC
    StoreItem<time_tools::TimeOffsetMinutes, defaults::timezone_minutes, journal::hash("Timezone Minutes")> timezone_minutes; // minutes offset for hour difference from UTC
    StoreItem<time_tools::TimeOffsetSummerTime, defaults::timezone_summer, journal::hash("Timezone Summertime")> timezone_summer; // Summertime hour offset

+    StoreItem<bool, defaults::bool_false, journal::hash("NTP via DHCP")> lan_ntp_via_dhcp; // use dhcp server for ntp


The lan prefix seems a bit wrong in both of these, as they are global settings.

it's being read from the lan area in the configuration ini, not sure where you would store it instead

The original implementation: - used dynamic allocation - did not lock the mutex on the first instantiation (get_handle was called before init_mutexes) - relied on hi2c.Instance, which was null before I2C initialization - had duplicit code (mutex inicialization) The code is still ugly, but hopefully at least a bit better. BFW-5274

Checking for the I2C handle was not enough. For example, the handle gets unlocked on I2C reinit, which kind of ruins things... BFW-5274

BFW-5313

The valued that is moved from is invalidated to not reset values at the end of its lifetime. BFW-5322

This should prevent noices when calibration X axis on XL and improve Tool Offset Calibrations, but only when run as part of selftest, not as gcode (will be done in separate commit/PR) BFW-5322

Small code cleanup. BFW-5322

BFW-5322

We shouldn't run any selftest with phasestepping enabled. To prevent running G425 with phase stepping we are disabeling phasestepping temporarily in the gcode it self instead of the selftest state machine. This improves Tool Offset Calibration results when running said calibration on already calibrated machine. The results are not perfect yet. But this change removes the major inaccuracies in the measurements. BFW-5276

We noticed shifts in measured data. Recomend reading BFW-5276 for more details, but in the end slowing down the parking speed helped a lot. BFW-5276

BFW-5335

Fill will be in the next commit BFW-5335

BFW-5335

The problem was in write_end_item calling migrate_bank. The function is called in store init, where migrate_bank would screw things. In other cases, the free space for the end item is checked in the parent calling functions. BFW-3553

…sensor_and_user_push__ask BFW-5361

This reverts commit f605a28bea662f02ec8d5121f4f57cee8ada774b.

BFW-5297

Copy pasted from standard printing screen BFW-5371

BFW-5305

BFW-5372

BFW-5346

BFW-5373

…pare manual ntp server ip entry. Add support for setting ntp ip addr manually via prusa_printer_settings.ini. Add ntp pool support.

This was referenced Jan 13, 2024

[FEATURE REQUEST] Mini - use a local time server #3523

Closed

[ENHANCEMENT] Option to set time manually #3671

Closed

bkerler force-pushed the ntp_setup branch from f7585a7 to 7eaa0ac Compare January 13, 2024 18:48

vorner reviewed Jan 16, 2024

View reviewed changes

lib/WUI/sntp/sntp_client.c Outdated Show resolved Hide resolved

lib/WUI/wui_api.cpp Outdated Show resolved Hide resolved

src/gui/MItem_lan.cpp Outdated Show resolved Hide resolved

bkerler force-pushed the ntp_setup branch 2 times, most recently from 73ae772 to b28ea62 Compare January 16, 2024 20:30

bkerler requested a review from vorner January 16, 2024 20:32

bkerler force-pushed the ntp_setup branch from b28ea62 to 3bec243 Compare January 16, 2024 21:50

thess mentioned this pull request Feb 8, 2024

[BFW-5926] Enable DNS for sntp and set default to prusa3d.pool.ntp.org #3730

Merged

bkerler force-pushed the ntp_setup branch from 0aebd3c to 2be9dbc Compare February 8, 2024 23:57

bkerler force-pushed the ntp_setup branch from 91eb222 to 178fdcc Compare February 10, 2024 19:53

bkerler force-pushed the ntp_setup branch 2 times, most recently from 5f8e662 to dfbb6c1 Compare February 12, 2024 10:46

vorner reviewed Feb 14, 2024

View reviewed changes

bkerler force-pushed the ntp_setup branch 5 times, most recently from 6b12551 to 89cea59 Compare February 18, 2024 14:57

bkerler requested a review from vorner February 18, 2024 14:57

CZDanol-prusa and others added 24 commits April 11, 2024 14:29

Improve Touchscreen_GT911::update_impl ISR check

f96adfd

Checking for the I2C handle was not enough. For example, the handle gets unlocked on I2C reinit, which kind of ruins things... BFW-5274

Hotfix: i2c::ChannelMutex

422e494

Header: larger 20x16 USB & stealth icons

7dd55a8

BFW-5313

PS: Make EnsureState movable

20cd3b2

The valued that is moved from is invalidated to not reset values at the end of its lifetime. BFW-5322

selftest: Disable phase stepping before running selftest

3f8d3de

This should prevent noices when calibration X axis on XL and improve Tool Offset Calibrations, but only when run as part of selftest, not as gcode (will be done in separate commit/PR) BFW-5322

cleanup: Remove unused includes and remove not needed virtual methods

27f4e03

Small code cleanup. BFW-5322

calib Z: Disable PS when running test with gcode

e73abe6

BFW-5322

toolchange: slowdown parking to make tool offset calib more accurate

80027e7

We noticed shifts in measured data. Recomend reading BFW-5276 for more details, but in the end slowing down the parking speed helped a lot. BFW-5276

Config store backend: END_ITEM_SIZE_WITH_CRC, BANK_HEADER_SIZE_WITH_CRC

cd12249

BFW-5335

Fix mistake in Backend::load_all

8801963

BFW-5335

journal::Backend: Introduce failing regression test

7c7fd66

Fill will be in the next commit BFW-5335

journal::Backend: More reasonable free bank space calculation

97769f4

BFW-5335

Load: Fix Response::Stop not working in LoadPhases_t::check_filament_…

f20608c

…sensor_and_user_push__ask BFW-5361

Revert "Disable input shaper menu"

ed9ffcc

This reverts commit f605a28bea662f02ec8d5121f4f57cee8ada774b.

Disable IS adjustments

10b36a3

BFW-5297

Confirm print stop on serial printing

eb6d96c

Copy pasted from standard printing screen BFW-5371

Fix stealth mode showing 0% with old gcode

f837441

BFW-5305

Fix screen_printing_data_t::w_etime_label not updating if value is N/A

b299ef1

BFW-5372

fix: Increase measurement task stack to run selftests in debug build

1a8d2ec

Tool mapping: Do not require double touch

3d0b8cf

BFW-5346

Translate FooterItemFinda values

0de68b8

BFW-5373

danopernis force-pushed the master branch from 7c651de to 0de68b8 Compare April 16, 2024 15:16

bkerler force-pushed the ntp_setup branch from f162dc5 to d993f6b Compare April 22, 2024 16:17

bkerler force-pushed the ntp_setup branch from 6a8bc7c to 53b3a46 Compare April 29, 2024 18:28

Add ntp server address to settings menu and ntp via dhcp support. Pre…

5a14456

…pare manual ntp server ip entry. Add support for setting ntp ip addr manually via prusa_printer_settings.ini. Add ntp pool support.

bkerler force-pushed the ntp_setup branch from 53b3a46 to 5a14456 Compare June 26, 2024 15:35

danopernis force-pushed the master branch from 37d88c7 to d97756f Compare August 13, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network/time: Add option to set ntp ip address or to set ntp via dhcp #3679

network/time: Add option to set ntp ip address or to set ntp via dhcp #3679

bkerler commented Jan 13, 2024 •

edited

Loading

bkerler commented Jan 13, 2024

vorner left a comment

thess commented Feb 8, 2024

bkerler commented Feb 8, 2024

bkerler commented Feb 8, 2024

thess commented Feb 9, 2024

bkerler commented Feb 10, 2024 •

edited

Loading

bkerler commented Feb 10, 2024

thess commented Feb 10, 2024

bkerler commented Feb 11, 2024

bkerler commented Feb 12, 2024

vorner left a comment

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 14, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 18, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 14, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 14, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 18, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

bkerler Feb 18, 2024

vorner Feb 14, 2024

bkerler Feb 14, 2024

network/time: Add option to set ntp ip address or to set ntp via dhcp #3679

Are you sure you want to change the base?

network/time: Add option to set ntp ip address or to set ntp via dhcp #3679

Conversation

bkerler commented Jan 13, 2024 • edited Loading

bkerler commented Jan 13, 2024

vorner left a comment

Choose a reason for hiding this comment

thess commented Feb 8, 2024

bkerler commented Feb 8, 2024

bkerler commented Feb 8, 2024

thess commented Feb 9, 2024

bkerler commented Feb 10, 2024 • edited Loading

bkerler commented Feb 10, 2024

thess commented Feb 10, 2024

bkerler commented Feb 11, 2024

bkerler commented Feb 12, 2024

vorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkerler commented Jan 13, 2024 •

edited

Loading

bkerler commented Feb 10, 2024 •

edited

Loading