-
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
Open
jamesmyatt
wants to merge
5
commits into
PaulStoffregen:master
Choose a base branch
from
jamesmyatt:feature/milliseconds
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e118fff
Provide access to left-over milliseconds in now function
jamesmyatt b774188
Add millisecond function
jamesmyatt 865b218
Update readme
jamesmyatt 586cddc
Add millisecond in keywords
jamesmyatt 1b57d09
Require user to request access to millisecond
jamesmyatt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
: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 inTime.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 toggleTIMELIB_ENABLE_MILLIS
from outside the code.Solutions are:
#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.#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.#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 includeTimeLib.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.