-
Notifications
You must be signed in to change notification settings - Fork 670
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.
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
Provide access to left-over milliseconds #93
base: master
Are you sure you want to change the base?
Conversation
33e5a20
to
1b57d09
Compare
any plan to merge it? |
This is something I'm looking for too, so please merge this! :) |
This would also meet my requirements. |
@@ -33,6 +33,7 @@ | |||
#include <WProgram.h> | |||
#endif | |||
|
|||
#define TIMELIB_ENABLE_MILLIS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather do this in Time.h
instead of hard-coding it in the compiled .cpp
-file and test via #if TIMELIB_ENABLE_MILLIS
:
#ifndef TIMELIB_ENABLE_MILLIS
#define TIMELIB_ENABLE_MILLIS true
#endif // #ifndef TIMELIB_ENABLE_MILLIS
This way, it is possible to pass -DTIMELIB_ENABLE_MILLIS=false
to the compiler without getting an annoying duplicate-definition warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this would work since the later code in Time.cpp
relies on all of the declarations in Time.h
. I think it would be better to #undef TIMELIB_ENABLE_MILLIS
if it's already defined at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will work, I am doing it in all my projects. The problem is that the .cpp
file is compiled in a different translation unit, so to change compiler flags inside it you have to either change it manually in the code (yuck!) or provide a global compiler flag. AFAIK it is not possible to tell the compiler to undo a specific #define
, so it is effectively impossible to toggle TIMELIB_ENABLE_MILLIS
from outside the code.
Solutions are:
- removing any
#define TIMELIB_ENABLE_MILLIS
from any files, keeping the#ifdef
checks and letting the user provide a command-line compiler flag. This however is inconvenient (if not impossible) for the Arduino IDE. - my suggestion: provide a boolean default in the header file (which is
#include
d in the source code file, thus causes a rebuild) and change the checks to#if
. This way, a global compiler flag can be used to explicitly enable or disable the feature (vs only being able to enable it as in solution 1). Still, the Arduino IDE problem. - Making this whole library a header-only library and using my suggestion (solution 2). This would enable users of the library to simply
#define TIMELIB_ENABLE_MILLIS true/false
before#include
ing the library to toggle the feature. No need for a global compiler flag, no problem in the IDE. That's what I am mostly doing. Has its own disadvantages. But that would be up to @PaulStoffregen to decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for avoiding the warning. However, I don't see the problem of forcing TIMELIB_ENABLE_MILLIS to be set in Time.cpp
. As you say it's a separate translation unit, so it doesn't change the functions available to the user when they include TimeLib.h
.
The purpose of this flag is to enable the declarations involving millis in TimeLib.h
so that an advanced user can say "I really want access to the leftover millis. I understand the risks and limitations. I promise I know what I'm doing and I won't complain". So with this solution, you can already just #define TIMELIB_ENABLE_MILLIS
in your sketch immediately before #include<TimeLib.h>
in your source code (this is what I do). No need for extra compiler flags.
Also I don't see why you would ever want to undefine it. Either you do nothing and the millis aren't available in your sketch, or you use the millis in your sketch so you define it in the sketch. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. You're right.
Funny way of using compiler flags to enable features. I guess, you could ditch all checks for TIMELIB_ENABLE_MILLIS
in the .cpp
-file altogether then, right? Can't undefine it via compiler command-line flags, hard-coded in the implementation file, feature is only enabled in the header file. So the milliseconds support is always compiled, right?
I guess one could leave it this way. Still, I prefer being explicit than implicit (i.e. being able to explicitly en-/disable some feature). But maybe that's just me. Still up to @PaulStoffregen to merge this into his library.
Thank you very much for this patch, merged manually, works like a charm! |
any chance to get this PR into the main release? |
any news / plans here? |
@cyberman54 , I think it's unlikely that this will get merged |
For anyone watching, it turns out the functions in the ESP8266 and ESP32 frameworks have functionality like this, e.g. |
👍 I want it! |
These are some minimal changes to provide access to the left-over millis from a call to now(). I know there's been lots of discussion about this, but I need it for my clock animations and I've included suitable warnings in the Readme. Furthermore, you have to enable access to the functions via a
#define
in your sketch, which I think is a pretty good compromise.I think this is better than the solutions in #11 and #45, since it will be consistent with the seconds output.
Datatype for the sysTimeMillis was selected to match prevMillis, which is uint32_t, rather than millis(), which is unsigned long.