Two receivers + why static definitions? about rc-switch HOT 3 CLOSED

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:

1. Sorry, but i have no idea how to do a pull request without forking, so I'm just going to attach my code here

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.

1. The code is a proof of concept, not too dirty, but can be much cleaner
2. I'm experienced in C/C++ but very new to Arduino

Then you should know that code duplication should be avoided ;-). Your code changes is doing exactly that, by duplicating the interrupt handler code.

1. Initially I wanted to go the way of non static, but after reviewing the code, it was much easier to just add another buffer and interrupt handler
2. No offence, but I wasn't convinced about the "access static" data and did a test, you can find it as non_static in my code. Interrupt handler is able to read + write to it, is my test correct?

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 non_static you give is about the second meaning, which restricts the visibility of a global variable to a file. So, there is no contradiction here.

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 static variables inside the interrupt handler into e.g. globals. But the problem I was alluding to is that there is no good way to write a single interrupt handler function, and then use that on multiple interrupts. In a sense, you precisely confirm this, as your code duplicates the interrupt handler function.

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).

1. Assuming my test is correct (aka interrupt handlers can access non static variables), I think the best way is to have 2 instances of rc-switch, 1 for each receiver, keeping as much code and variables static while the buffers non static
2. My earlier encounter with static definitions on Arduino was to prevent memory fragmentation, this was certainly a valid consideration
3. I'm not sure if 2 or more instances will lead to any memory fragmentation or other issues

I perceive two primary problems with your code:

1. It forces a second buffer onto everybody, even people who don't need it (and who need the memory for other things).
2. It duplicates the interrupt handler code.

Problem 1 could e.g. be addressed by a compile time switch, such as an optional #define USE_SECOND_RECEIVE_HANDLER at the top of RCSwitch.h, to enable/disable compilation of this feature. Or perhaps even better: #define MAX_NUM_OF_RECEIVERS 2 and then allow setting it to 0 (= disable receiver completly), 1 (= current behavior), 2 (your patch), and possibly even higher.

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:

struct InterruptHandlerState {
  unsigned int changeCount;
  unsigned long lastTime;
  unsigned int repeatCount;
};

void RCSwitch::handleInterruptGeneric(InterruptHandlerState &state, unsigned int *timings) {
  ...
}

#if MAX_NUM_OF_RECEIVERS >= 1
void RECEIVE_ATTR RCSwitch::handleInterrupt0() {
  static InterruptHandlerState state;
  handleInterruptGeneric(state0, RCSwitch::timings0);
}
#endif

#if MAX_NUM_OF_RECEIVERS >= 2
void RECEIVE_ATTR RCSwitch::handleInterrupt1() {
  static InterruptHandlerState state;
  handleInterruptGeneric(state, RCSwitch::timings1);
}
#endif

from rc-switch.

@fingolfin
You are right, I retested by placing non_static in the RCSwitch.h in order to use this->non_static and it doesn't compile anymore. I know the code is not clean, but it was the quickest way to meet my use case and anyone who needs exactly two receivers. :)

from rc-switch.

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.

from rc-switch.