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.

Sign up for GitHub

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

Jump to bottom

Add SNTP from DHCP #2798

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Add SNTP from DHCP #2798

wants to merge 19 commits into from

Conversation

networkfusion
Copy link
Member

@networkfusion networkfusion commented Oct 20, 2023 •
edited
Loading

Description

  • Add ability to retrive time from DHCP.
  • ChibiOS & TI: Ensure the SNTP service is stopped before configure.
  • ChibiOS: Set default lwIP MAC to developer ID.
  • ChibiOS: Initalize SNTP properly.
  • Change default servers to seperate concerns.

Motivation and Context

  • Improves ability to run on more networks.
  • Stops ntp server from being swamped by requests (if possible).

How Has This Been Tested?

Tested on an STM32F769I

Before, devices failed to get time from network helper in a reasonable timeframe.
It is now pretty much instantanious for me. Edit: spoke too soon.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

networkfusion and others added 3 commits October 20, 2023 17:54
@networkfusion
Add sntp from dhcp
30400c0 
Ensure the SNTP service is stopped before configure.
Change default servers to seperate concerns.
@nfbot
Code style fixes
026c57b 
Automated fixes for code style.
@networkfusion
Merge pull request #4 from nanoframework/nfbot/clang-format-fix/a8d93…
1e919d8 
…bea-ae52-4668-9824-bc42dd0454a3

Code style fixes for nanoframework/nf-interpreter PR#2798
@networkfusion networkfusion added ⚠️ DO NOT MERGE ⚠️ Platform: STM32 Everything related specifically with ChibiOS platform labels Oct 20, 2023
@networkfusion
Copy link
Member Author

networkfusion commented Oct 20, 2023 •
edited
Loading

All of the changes in this PR seem to help, but still suffering from the underlying issue sometimes (what ever it may be).
Possibly a callback when the ip or time is set because it seems to always work on second deploy.

Hopefully it will help future debugging.

josesimoes
josesimoes reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Please see my comments.
If we're going forward with this (which I agree that's a good improvement) for consistency sake, it has to behave the same way in the other platforms.
Let me know if you want me to take care of that.

targets/ChibiOS/_Lwip/nf_lwipthread.h Outdated
@@ -91,42 +91,42 @@
* @brief MAC Address byte 0.
*/
#if !defined(LWIP_ETHADDR_0) || defined(__DOXYGEN__)
#define LWIP_ETHADDR_0 0xC2
#define LWIP_ETHADDR_0 0x00
Copy link
Member

Choose a reason for hiding this comment

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

Why the change in the default MAC address? This is was using ST default for development devices.

Copy link
Member Author

@networkfusion networkfusion Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure??? all of the STM targets set the dev mac to:
image
In targetHAL_ConfigurationManager.cpp Which is what I have adjusted to...

targets/ChibiOS/_Lwip/nf_lwipthread.c Outdated
static void initialize_sntp()
{
sntp_stop();
sntp_setoperatingmode(SNTP_OPMODE_POLL);
Copy link
Member

Choose a reason for hiding this comment

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

According to lwIP docs, this is already the default, so there's no need for this call.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I was just copying the mechinisum used by ESP32. Also, the particular line you are commenting on was already present (just moved so it was not duplicated).

targets/ChibiOS/_Lwip/nf_lwipthread.c Outdated Show resolved Hide resolved
networkfusion and others added 2 commits October 23, 2023 10:59
@networkfusion @josesimoes
Update targets/ChibiOS/_Lwip/nf_lwipthread.c
db7cb5d 
Co-authored-by: José Simões <[email protected]>
@nfbot
Code style fixes
2bc94a1 
Automated fixes for code style.
@networkfusion
Copy link
Member Author

Please see my comments. If we're going forward with this (which I agree that's a good improvement) for consistency sake, it has to behave the same way in the other platforms. Let me know if you want me to take care of that.

Yes, please adjust what you see fit to bring inline with other platforms etc.

@networkfusion
Merge pull request #5 from nanoframework/nfbot/clang-format-fix/c7636…
af101c9 
…ad3-3634-462a-9d4d-d9e58e668014

Code style fixes for nanoframework/nf-interpreter PR#2798
@networkfusion
Move SNTP_GET_SERVERS_FROM_DHCP to sntp options
cfcdc0f 
Add to other platforms
@networkfusion
Merge branch 'main' into chibios-sntp
58874d0
@networkfusion
Copy link
Member Author

@josesimoes I had a bit of time, so have "attempted" to add to other platforms.

nfbot and others added 2 commits October 23, 2023 15:27
@nfbot
Code style fixes
025c270 
Automated fixes for code style.
@networkfusion
Merge pull request #6 from nanoframework/nfbot/clang-format-fix/9ba74…
fab91bc 
…a79-253e-4c86-86fb-6825f847757f

Code style fixes for nanoframework/nf-interpreter PR#2798
@nanoframework nanoframework deleted a comment from nfbot Oct 23, 2023
@nanoframework nanoframework deleted a comment from nfbot Oct 23, 2023
@networkfusion networkfusion changed the title Add SNTP from DHCP (ChibiOS) Add SNTP from DHCP Oct 23, 2023
@networkfusion networkfusion marked this pull request as ready for review November 1, 2023 15:53
@networkfusion networkfusion added Platform: ESP32 Everything related specifically with ESP32 platform Platform: TI-SimpleLink labels Nov 16, 2023
@networkfusion
move initialize_sntp()
9558d4c 
make static again.
move above all functions that use it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josesimoes josesimoes josesimoes left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Platform: ESP32 Everything related specifically with ESP32 platform Platform: STM32 Everything related specifically with ChibiOS platform Platform: TI-SimpleLink Type: enhancement ⚠️ DO NOT MERGE ⚠️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@networkfusion @josesimoes @nfbot