Skip to content

Commit

Permalink
Bugfix for displaying NTP time
Browse files Browse the repository at this point in the history
  • Loading branch information
@fredlcore
fredlcore committed Nov 15, 2023
1 parent eade926 commit 0f636be
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion BSB_LAN/BSB_LAN.ino
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4695,7 +4695,7 @@ void SetDateTime() {
tzset();

if(getLocalTime(&timeinfo)){
printFmtToDebug(PSTR("Date and time acquired: %02d.%02d.%02d %02d:%02d:%02d\n"), timeinfo.tm_mday, timeinfo.tm_mon, timeinfo.tm_year-100, timeinfo.tm_hour, timeinfo.tm_min, timeinfo.tm_sec);
printFmtToDebug(PSTR("Date and time acquired: %02d.%02d.%02d %02d:%02d:%02d\n"), timeinfo.tm_mday, timeinfo.tm_mon+1, timeinfo.tm_year-100, timeinfo.tm_hour, timeinfo.tm_min, timeinfo.tm_sec);
// setTime(timeinfo.tm_hour, timeinfo.tm_min, timeinfo.tm_sec, timeinfo.tm_mday, timeinfo.tm_mon, timeinfo.tm_year); // not sure if setTime is necessary here or if configTime already takes care of that?
return;
} else {
Expand Down

11 comments on commit 0f636be

@fredlcore
Copy link
Owner Author

Choose a reason for hiding this comment

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

@DE-cr: I don't have the means to test SD card logging and displaying, could you please test the newest release and check if the NTP-acquired date and time also reflect correctly in your logging code? Or were you using UTC and this doesn't affect your code?

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023

Choose a reason for hiding this comment

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

I'll look into it.

Btw, I was just about to try and figure out why /R doesn't work as it used to: the parameter name isn't displayed with the reset value anymore.

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023

Choose a reason for hiding this comment

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

Shouldn't line #7593 read #if defined(ESP32) && defined(USE_NTP) instead of #if defined(ESP32)?
(Btw, I don't see a reason why I should use NTP on my system, so I'll disable it after testing.)

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't line #7593 read #if defined(ESP32) && defined(USE_NTP) instead of #if defined(ESP32)?

Ok, maybe not necessarily. However, I don't get why ESP32 gets a special routine there then.

@fredlcore
Copy link
Owner Author

Choose a reason for hiding this comment

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

Just try and compile it under Arduino then. Even though I'm not actively adding functionality for the Arduino anymore (at least not if it requires complex code), I still want it to compile without errors.

@fredlcore
Copy link
Owner Author

Choose a reason for hiding this comment

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

/R works as expected, returning the reset value. Don't know what else you mean, last code changes on that part were three years ago. If you see a bug, please report it via the proper channels.

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023

Choose a reason for hiding this comment

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

Did that, and the testing (flashed the new firmware, and checked if my running log is continued as expected): seems to be okay.

As I don't want to use #define USE_NTP on my system, could you please change line 7593 as suggested by me above? Otherwise I don't feel confident syncing my fork with the bsb-lan master repository.

With the parameter(s) I've tried today, /R doesn't work correctly for me, e.g.:
grafik
(This is running the current bsb-lan master code.)

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

P.s. ...or is this indeed the expected behaviour? I kind of remembered seeing the parameter name to the left of the reset value in /R output. (P.p.s. ...which I would consider very helpful.)

P.p.p.s. It is included in the debug log:

GET /R1060 HTTP/1.1
URL: /R1060
LAN->HEIZ QRV      3D2E0614 
DC C2 00 0B 0F 3D 2E 06 14 54 92
HEIZ->LAN ARV 1060.0 Heizkreis 2 - Raumtemperatur-Schaltdifferenz Ausschaltpunkt: --- °C

@fredlcore
Copy link
Owner Author

Choose a reason for hiding this comment

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

Please don't hijack this conversation with other matters. If you can show that pieces of code have been dropped, then I can check for that, but as I said, GitLens tells me that the last changes were three years ago already. And that's it here for that matter.
And where exactly do you want me to change the USE_NTP condition? You can make comments right in the code here on GitHub. But apart from that, the NTP-related code is already enclosed in SetDateTime(), so it won't be executed if you don't have USE_NTP enabled. All other instances are just using the tm struct, so they are independent of NTP as it is still possible to retrieve the time from the heater. Why someone wouldn't want to use NTP (if only to periodically set the correct time in the heater) beats me, since we have more or less already given up on the requirement that no outside connection is required (for example with your graph extension). But as I said, if you don't want to use it, you don't have to.

@DE-cr
Copy link
Contributor

@DE-cr DE-cr commented on 0f636be Nov 16, 2023

Choose a reason for hiding this comment

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

/DG can run without outside connection, as I use it on my system. This is the relevant excerpt from BSB_LAN_config.h:

// Log file plotting uses two JavaScript libraries, which are usually loaded from the web.
// Should you prefer to use local copies instead, put the files from the data subdirectory onto your
// bsb-lan unit's SD card (*), and provide their path names here. For browsers that support gzip
// compression (what browser doesn't?), it is sufficient to copy the *.gz file versions to your
// bsb-lan unit, but omit the ".gz" from the path names you put into the following lines!
// (*) In case you're using an ESP32's internal memory instead of an SD card, use
// https://github.com/lorol/arduino-esp32littlefs-plugin to upload the files.
// For local use of these libraries to work, "#define WEBSERVER" is also required!
#define D3_LIBRARY_PATH "/d3.js"
#define C3_LIBRARY_PATH "/c3.js"

@fredlcore
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, I only said that the requirement that no outside connection can be required has been dropped.
But since you are again not replying to the matter at question, just forget about it,I don't have time for this.

Please sign in to comment.