Remove "Epoch" that cause compliation errors, add example for Resets and Watchdog #237
Conversation
Bringing up to date with release1.1.2
I hate to undo even a little bit of Adam's good work, but in the interests of a robust core, I think it's best the references to time.h are removed, and the Epoch set/get along with it. RTC.cpp included <time.h>, which caused a compilation error when these are also included in code: #include <Time.h> #include <TimeLib.h> Some possible issues with Window lack of case sensitivity, refer here: https://forum.arduino.cc/index.php?topic=451360.0 Also with time.h (or Time.h?) being an optional extra install library to the Arduino IDE, best to leave it out of the core, especially if it can cause compilation errors.
|
Ah, the good ol' time.h issue: mikalhart/IridiumSBD#16 If you're using Paul Stoffegren's Time library, you'll need to rename or delete the <Time.h> file. Paul made the decision to keep this file named as such, despite users experiencing the problem you've described.
Cheers, P.S. I'd suggest moving your comments directly into the PR to help SparkFun more easily review the discussion surrounding any changes to be made. |
|
Yes, the time.h issue seems like a case of "that old chestnut". I was using the "Time" library (v1.6.0) by Michael Margolis, which was just the one that seemed to be most legit in the Arduino IDE Add Library dialogue. I won't be offended if my suggested change is outvoted and knocked back, but figured on making it easy to be accepted (or not). Fair call on moving comments directly to the PR, I've edited it to add them. Still getting the hang of Github and how the collaboration works, I had to do some vehement swearing to get it working this morning. |
|
Hi @stephenf7072, Michael Margolis's and Paul Stoffegren's Time libraries are one and the same. I believe the plan is for future versions of the library to remove <Time.h>. There's also a current open issue regarding the case-sensitivity issue it's causing. Given that the issue is with the Time library, and not the C++ <time.h> header, I don't believe there are any issues with having the epoch code remain. Cheers, |
There was a problem hiding this comment.
Thanks a bunch @stephenf7072 for this PR, we know how much work it takes to do this. With that I would like to request a few changes.
- I see the need for a WDT library. This would help hide some of the scarier things like HAL calls and C programming nitty-gritty from entry-level users. It would also afford the opportunity to organize several small examples that work up to the grand finale that is the ResetsAndWatchdog example ;)
- I do not believe that this PR should be modifying the RTC library. If those changes are proposed then they should be in a separate PR, but after reviewing the situation with Time.h it is my opinion that this core should maintain the standard
<time.h>and look to the future where Paul resolves the case sensitivity issue on his end. (This may a good reminder for us all to be more cognizant of using relative includes too - not that it applies in this case but it recently came up in an Arduino PR)
| @@ -0,0 +1,196 @@ | |||
| /* Author: Stephen Fordyce (adapted from WDT example code by Adam Garbo - https://forum.sparkfun.com/viewtopic.php?f=169&t=52431&p=213296&hilit=watchdog#p213296) | |||
There was a problem hiding this comment.
This example includes a lot of great functionality!
It is fairly overwhelming as the only WDT example - it would be great to have a WDT library with several bite-sized examples
There was a problem hiding this comment.
Hi Owen, sounds like I'm outvoted on the RTC library, no problem. Also noted the preference for doing separate smaller PRs for different features (makes sense).
Github implies you've requested a change on line 1 (or line 196?) of Example8_ResetsAndWatchdog.ino but I can't see what it is - can you explain what's going on? (still a bit of a Github numpty here)
I agree a WDT library and some smaller examples would be better, but I've expended my energy budget on this for the moment (no problem from me if someone wants to pick it up and run with it). I deliberately structured the example as it is, so it's also an all-in-one-place reference for what HAL calls are involved and the lower level stuff. Saving advanced user from having to pick through various files, and giving less advanced users a good chance at copy/pasting functionality. Ie. it should stand alone, and can be replaced/supplemented by library examples later. I figured better to have this in the core as an example than posted on a forum.
Do you want me to cancel this multi-faceted pull request and do another one just for the Reset/Watchdog example?
Cheers,
Stephen
There was a problem hiding this comment.
Yes, that would be great. Thank you. I agree, we can build smaller examples over time but for now lets get it in here!
|
Closing pull request to submit another with only the Reset/Watchdog example. |
Each of these are detailed in the commits, repeated here:
Add an example to demonstrate use of the watchdog timer, also different ways of resetting the Artemis as well as how to determine the last reset type
Removed code using <time.h> non-core library (ie. Epoch stuff)
I hate to undo even a little bit of Adam's good work, but in the interests of a robust core, I think it's best the references to time.h are removed, and the Epoch set/get along with it.
RTC.cpp included <time.h>, which caused a compilation error when these are also included in code:
#include <Time.h>
#include <TimeLib.h>
Some possible issues with Window lack of case sensitivity, refer here: https://forum.arduino.cc/index.php?topic=451360.0
Also with time.h (or Time.h?) being an optional extra install library to the Arduino IDE, best to leave it out of the core, especially if it can cause compilation errors.