-
Notifications
You must be signed in to change notification settings - Fork 652
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
Two receivers + why static definitions? #63
Comments
Thanks. All in all, I see you did not thing special here, so I am no longer surprised. Rather, you simply replaced the existing hard-coded limitation of a single receiver by another hard-coded limitation (exactly two receivers). That is of course not hard to achieve. The difficulties I was alluding to rather are in making the code such that you can simply make one, two or three RCSwitch instances in order to use one, two or three receivers. Anyway, let me reply to some specific points:
Yes, you need to make a fork in order to create a pull request. That's a feature, not a bug :-). It also makes it possible to properly review your changes, as opposed to just having to look at a big blob of code.
Then you should know that code duplication should be avoided ;-). Your code changes is doing exactly that, by duplicating the interrupt handler code.
Sorry, I guess I unclear in what I wrote: First off, note that static has two meanings in C/C++: the one I am referring to is the use of static variables inside a function; the example with Secondly, my point simply was that interrupt handlers can only access global/static variables (and of course anything reachable from that). Now, it would easily be possible to turn the This is a problem for a library, which now has to restrict itself to a hard coded limit of the number of interrupts it supports (currentl 1 in the case of RCSwitch). (Your patch raises that limit to 2, but it still is hard coded).
I perceive two primary problems with your code:
Problem 1 could e.g. be addressed by a compile time switch, such as an optional Problem 2 could be solved by factoring out the state of the interrupt handler into a struct, and the code into a separate function. This then also makes it easy to support more than two receivers, if desired. Here is a rought sketch of how that could look like:
|
@fingolfin |
Of course! Everybody does that, me certainly included :-). Thank you for your input, though, it made me think about this again: While there may be no "perfect" solution, one could still add a working solution which at least helps people who need two receivers; and if done right, it should not "harm" people who need no or just a single receiver. |
@fingolfin
non_static
in my code. Interrupt handler is able to read + write to it, is my test correct?Finally, thanks for this library, I having a fantastic time turning on/off electricity :)
The text was updated successfully, but these errors were encountered: