cyborg5 / IRLib

An Arduino library for encoding and decoding infrared remote signals
Other
213 stars 76 forks source link

Interrupt based ir receives a little buggy #16

Closed ElectricRCAircraftGuy closed 6 years ago

ElectricRCAircraftGuy commented 8 years ago

Hi Chris, great library! I really like the idea of timer-free receives. However, using your IRrecvPCI is a little buggy. I frequently get erroneous reads because packets are partial and don't contain all the pulses. I see you are missing atomic access guards when accessing your volatile variables outside ISRs, and there are some timing issues with attaching and detaching the interrupt I think. I'll see if I can make it work better and then I'll do a pull request. If you like what I'm able to come up with you can incorporate it into your master.

cyborg5 commented 8 years ago

I look forward to seeing what you have. I'm not sure I understand what precautions I need to take when accessing volatile variables. The main problem I found with the PCI receiver is that it doesn't always recognize the gap between multiple codes. The problem is that it doesn't recognize the end of the final Space until the beginning of the next Mark arrives. It tries to detect a timeout but it doesn't always catch it if the next code comes in too quickly. By missing the first mark of the second frame that second code doesn't get detected properly.

From: Gabriel Staples [mailto:notifications@github.com] Sent: Monday, January 25, 2016 10:33 AM To: cyborg5/IRLib IRLib@noreply.github.com Subject: [IRLib] Interrupt based ir receives a little buggy (#16)

Hi Chris, great library! I really like the idea of timer-free receives However, using your IRrecvPCI is a little buggy I frequently get erroneous reads because packets are partial and don't contain all the pulses I see you are missing atomic access guards when accessing your volatile variables outside ISRs, and there are some timing issues with attaching and detaching the interrupt I think I'll see if I can make it work better and then I'll do a pull request If you like what I'm able to come up with you can incorporate it into your master

— Reply to this email directly or view it on GitHub https://github.com/cyborg5/IRLib/issues/16 . https://github.com/notifications/beacon/ADcguvUWJsL3L6Drd8E9v6YYG_0MXYKOks5pdje_gaJpZM4HLs3c.gif

ElectricRCAircraftGuy commented 8 years ago

This is insightful. How are you detecting the end of a frame for the 50us interrupt-based polling technique (IRrecv) then?

ElectricRCAircraftGuy commented 8 years ago

Ps here's how to protect against interrupt-based variable corruption when accessing volatile variables outside an ISR:

byte SREG_backup = SREG; //back up interrupt register
noInterrupts(); //turn interrupts off
//access volatile variables here
SREG = SREG_back; //restore interrupt state (off of it was off before, on if it was on before)
cyborg5 commented 8 years ago

If a particular length of time occurs with no new marks, it concludes that the frame has finished. Basically it is a timeout that if a certain number of 50 µs ticks go by with no new signal then we must be done. But when using the pin change interrupt we only look at the input when the pin changes. The pin goes low after the last mark and stays low. The software is waiting for it to go high again so we can say "aha… That is the end of the blank space." It then processes that frame but is not yet prepared to receive the start of the new frame. We can poll the input during the normal Arduino loop function to try to detect how long it's been since the last pin change and determine that there is a timeout. But depending on what else you have going on in your main loop a new signal could start. It would generate the pin change interrupt which would cause whatever you're doing by then it's too late. We don't want to ever see that rising signal on the second frame until we completely processed the first frame. You have to play around with what is or is not a timeout in the main loop. Basically each of the three methods of receiving signals has its own strengths and weaknesses. I've been tied up with other projects recently but I will be getting back into IR very soon with a major rewrite that I will call version 2.0. Could you please send me email directly at cy_borg5@cyborg5.com mailto:cy_borg5@cyborg5.com and give me a little more detail about why I have to be careful with volatile variables. Perhaps show me some sample it isn't working because of this.

From: Gabriel Staples [mailto:notifications@github.com] Sent: Monday, January 25, 2016 3:00 PM To: cyborg5/IRLib IRLib@noreply.github.com Cc: Chris Young cy_borg5@cyborg5.com Subject: Re: [IRLib] Interrupt based ir receives a little buggy (#16)

This is insightful. How are you detecting the end of a frame for the 50us interrupt-based polling technique (IRrecv) then?

— Reply to this email directly or view it on GitHub https://github.com/cyborg5/IRLib/issues/16#issuecomment-174640897 . https://github.com/notifications/beacon/ADcguho_PRVu1d-A_JLp_C2VnD-UqztCks5pdnY5gaJpZM4HLs3c.gif

ElectricRCAircraftGuy commented 8 years ago

Hey thanks for the info! Very helpful. I'll come up with something. Ive got some experience using pin change type interrupts to decode RC PPM signals. That will help me here. And I can give some more info about volatile later too.

ElectricRCAircraftGuy commented 8 years ago

PS. not using atomic access guards isn't your main problem here. It's something that would only occasionally cause a problem. I'll do some rewriting of the algorithms to solve the main problems you describe above, and I'll add atomic access guards while I'm at it.

Basically, if you don't ensure atomic access, it is possible to read only one or two or three bytes of, per say, a 4-byte variable, when all of a sudden an interrupt fires and updates that variable. Then, your main codes picks back up reading the rest of the bytes of the multi-byte variable. Now you could have some bytes from the old value and some bytes from the new value, which means your value is totally wrong. Let's look at a 2-byte example. Let's say you have decimal 65535, or in binary, 11111111 11111111. You read the most-significant byte as 11111111. Now, the interrupt fires and increments the 2-byte value to 00000000 00000000. Then, you return to the main code and read the rest of the value, this time the least-significant byte, as 00000000. Your result is 11111111 00000000, or 65280, when in fact it should be decimal 0. Here, you have non-atomic access, meaning the multi-byte variable was accessed in pieces, 1 byte at a time. To force atomic access, or access where this type of byte-level corruption to variables can not occur, you must disable interrupts while accessing the variable, then restore the interrupt state when done. This type of problem is not seen very often, but it is seen, and you must always guard against it. You can have very bad results in some cases, and no matter what, we want our code to function as desired all of the time, not most of the time.

I'll send you an email too. Meanwhile, I'll work on the code and see what I can do to improve the algorithm and solve the main problem at hand.