Allow the message timeout value to be set at run-time. #294
Conversation
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
roidayan
approved these changes
Aug 4, 2017
src/IRrecv.cpp
Outdated
| irparams.recvpin = recvpin; | ||
| irparams.bufsize = bufsize; | ||
| // Ensure we are going to be able to store all possible values in the | ||
| // capture buffer. | ||
| irparams.timeout = std::min(timeout, (uint8_t) (RAWTICK * UINT16_MAX / 1000)); |
There was a problem hiding this comment.
I suggest to have a definition for the maximum allowed. I.e. MAX_TIMEOUT
There was a problem hiding this comment.
MAX_TIMEOUT_MS for consistency
src/IRrecv.cpp
Outdated
| DPRINTLN(")]"); | ||
| // We really should never get a value of 0, except as the last value | ||
| // in the buffer. If that is the case, then assume infinity and return true. | ||
| if (measured == 0) return true; | ||
| return measured >= ticksLow(std::min(desired, TIMEOUT_MS * 1000), | ||
| return measured >= ticksLow(std::min(desired, | ||
| (uint32_t) (irparams.timeout * 1000)), |
There was a problem hiding this comment.
Isn't there MS_TO_USEC ? If not can add for readability and it doesn't cost
|
Great suggestions. Will do.
…On Fri., 4 Aug. 2017, 11:04 pm Roi Dayan, ***@***.***> wrote:
***@***.**** approved this pull request.
Beside my comment about definition s hta will add readability looks good
to me.
------------------------------
In src/IRrecv.cpp
<#294 (comment)>
:
> irparams.recvpin = recvpin;
irparams.bufsize = bufsize;
+ // Ensure we are going to be able to store all possible values in the
+ // capture buffer.
+ irparams.timeout = std::min(timeout, (uint8_t) (RAWTICK * UINT16_MAX / 1000));
I suggest to have a definition for the maximum allowed. I.e. MAX_TIMEOUT
------------------------------
In src/IRrecv.cpp
<#294 (comment)>
:
> irparams.recvpin = recvpin;
irparams.bufsize = bufsize;
+ // Ensure we are going to be able to store all possible values in the
+ // capture buffer.
+ irparams.timeout = std::min(timeout, (uint8_t) (RAWTICK * UINT16_MAX / 1000));
MAX_TIMEOUT_MS for consistency
------------------------------
In src/IRrecv.cpp
<#294 (comment)>
:
> DPRINTLN(")]");
// We really should never get a value of 0, except as the last value
// in the buffer. If that is the case, then assume infinity and return true.
if (measured == 0) return true;
- return measured >= ticksLow(std::min(desired, TIMEOUT_MS * 1000),
+ return measured >= ticksLow(std::min(desired,
+ (uint32_t) (irparams.timeout * 1000)),
Isn't there MS_TO_USEC ? If not can add for readability and it doesn't cost
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#294 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMInDM1Z-3wM1Fokvv6S_SqFnsamfF5Mks5sUxbAgaJpZM4OtYbT>
.
|
crankyoldgit
force-pushed
the
set_timeout
branch
from
521ac2b to
f7ce1f6
Compare
|
Changes done. |
roidayan
reviewed
Aug 5, 2017
src/IRrecv.h
Outdated
| @@ -18,6 +18,7 @@ | |||
| #define HEADER 2U // Usual nr. of header entries. | |||
| #define FOOTER 2U // Usual nr. of footer (stop bits) entries. | |||
| #define OFFSET_START 1U // Usual rawbuf entry to start processing from. | |||
| #define MS_TO_USEC 1000U // Convert milli-Seconds to micro-Seconds. | |||
There was a problem hiding this comment.
I suggest here
#define MS_TO_USEC(x) (x *1000)
Then usage is
MS_TO_USEC(irparams.timeout)
crankyoldgit
force-pushed
the
set_timeout
branch
from
f7ce1f6 to
050de44
Compare
roidayan
approved these changes
Aug 5, 2017
src/IRrecv.cpp
Outdated
| @@ -427,17 +432,18 @@ bool IRrecv::matchAtLeast(uint32_t measured, uint32_t desired, | |||
| DPRINT(". Matching: "); | |||
| DPRINT(measured); | |||
| DPRINT(" >= "); | |||
| DPRINT(ticksLow(std::min(desired, TIMEOUT_MS * 1000), tolerance)); | |||
| DPRINT(ticksLow(std::min(desired, (uint32_t) MS_TO_USEC(irparams.timeout)), | |||
There was a problem hiding this comment.
Isn't casting redundant here?
There was a problem hiding this comment.
No. Lint and compiler error/warnings otherwise. The later parameter defaults to an int.
src/IRrecv.cpp
Outdated
| return measured >= ticksLow(std::min(desired, TIMEOUT_MS * 1000), | ||
| tolerance); | ||
| return measured >= ticksLow( | ||
| std::min(desired, (uint32_t) MS_TO_USEC(irparams.timeout)), tolerance); |
There was a problem hiding this comment.
Isn't casting redundant here?
There was a problem hiding this comment.
No. Lint and compiler error/warnings otherwise. The later parameter defaults to an int.
There was a problem hiding this comment.
Ok so all looks good to me.
There was a problem hiding this comment.
But now with the macro/function, it doesn't. So castings removed.
There was a problem hiding this comment.
Updated. 3rd time is the charm. I hope.
Ensure the timeout value won't allow a value larger than we can store.
crankyoldgit
force-pushed
the
set_timeout
branch
from
050de44 to
48168c2
Compare
roidayan
approved these changes
Aug 5, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ensure the timeout value won't allow a value larger than we can store.