crankyoldgit / IRremoteESP8266

Infrared remote library for ESP8266/ESP32: send and receive infrared signals with multiple protocols. Based on: https://github.com/shirriff/Arduino-IRremote/
GNU Lesser General Public License v2.1
2.97k stars 832 forks source link

Lasertag Protocol Parameters #1360

Closed laserir closed 3 years ago

laserir commented 3 years ago

Dear all, I tried a lot of parameters for the Lasertag protocol for almost a year now and have figured out what will result in almost 100% correct recognition. I suggest to do the following changes inside the ir_Lasertag.ccp:

const uint32_t kLasertagMinGap = 100000; const uint16_t kLasertagDelta = 165;

Change this line: if (actual_bits < nbits) return false; // Less data than we expected. to this code (from the beta):

if (actual_bits < nbits) {  // Less data than we expected.
        for(int mbcount = 1; mbcount <= (nbits - actual_bits); mbcount++) {
            data <<= 1; //Add 0
        }
    }

Lastly, strict parameter in the decodeLasertag function should default to false!

NiKiZe commented 3 years ago

Where is the data that you base this on? We prefer dumps from the recommended sketch.

laserir commented 3 years ago

I was the one bringing up the protocol, see https://github.com/crankyoldgit/IRremoteESP8266/issues/366

Dumps and everything is in there.

crankyoldgit commented 3 years ago

const uint32_t kLasertagMinGap = 100000;

FYI, currently it's set to:

const uint32_t kLasertagMinGap = kDefaultMessageGap;  // Just a guess.

and kDefaultMessageGap is 100000 already. So no need to change that.

crankyoldgit commented 3 years ago

Change this line: if (actual_bits < nbits) return false; // Less data than we expected. to this code (from the beta):

if (actual_bits < nbits) {  // Less data than we expected.
      for(int mbcount = 1; mbcount <= (nbits - actual_bits); mbcount++) {
          data <<= 1; //Add 0
      }
  }

This doesn't seem right. That line is/was there to check that we got the expected number of bits for the protocol. IIRC it's a fixed size nr of encoded bits, the number of pulses is not fixed due to the encoding type.

I went back and reviewed the thread in #366 , I believe I requested captures (via IRrevDumpV2) for the messages that don't currently match via that decoder. Without them, I can't be sure what is wrong, and I can't add a test-case to make sure any future code changes continue to match those (new) messages.

So, can you please supply captures of the messages that don't work.

Lastly, strict parameter in the decodeLasertag function should default to false!

I'm not convinced that is correct (see above)

Re: Changing the delta value to 165 I'll look at doing that shortly.

laserir commented 3 years ago

Due to lockdown I currently don't have access to the equipment, but will try to put up a test to see which signals are read and which are not with the current version vs. my suggested code change. Thanks so far!

NiKiZe commented 3 years ago

What we are requesting is the raw data which should be the same regardless of using old or new code.

That raw data will be used to create unit tests to be sure that any changes are needed and valid. As well to make sure that they stay valid in all future versions.

crankyoldgit commented 3 years ago

const uint16_t kLasertagDelta = 165; has been updated in PR #1362 / Branch https://github.com/crankyoldgit/IRremoteESP8266/tree/lasertag_improvements

Can you please download and confirm it works as you expect? (On it's own, it's a fairly innocuous change, so it really doesn't need verification)

crankyoldgit commented 3 years ago

Due to lockdown I currently don't have access to the equipment, but will try to put up a test to see which signals are read and which are not with the current version vs. my suggested code change. Thanks so far!

Thanks. Looking forward to the captures so we can progress this issue.

laserir commented 3 years ago

So, luckily I could contact a technician and perform some tests. It seems that the original code performs well under "lab conditions", meaning if the distance is not too far and the room is not foggy (like the lasertag arena normally would be). But in the arena, the bigger the distance and proportion of haze, the signals will less likely be read. We compared the original code vs. the suggested code block (kLasertagDelta in both cases = 165) and found that with the change (and tbh I have no idea what it does, its just copied from the beta) the recognition rate over distance and in haze is much better, like 3-4 times the distance is possible for correct reads.

crankyoldgit commented 3 years ago

I get why you may want that ability, and I'm happy to try to add that in a safe way. If you can supply a capture in the above scenario, we might be able to program/organise something for you.

laserir commented 3 years ago

We send back these raw codes, which show the behavior: https://github.com/Arduino-IRremote/Arduino-IRremote/issues/532#issuecomment-336461835

Would you mind to explain in short words why that code block would help / what it does?

crankyoldgit commented 3 years ago

We send back these raw codes, which show the behavior: Arduino-IRremote/Arduino-IRremote#532 (comment)

As far as I remember, our library decodes those codes successfully. Is that not the case?

Would you mind to explain in short words why that code block would help / what it does?

The shortest/simplest I can say what that code block does is:

"When the message is shorter than what is expected, it adds zeros to the end of the code to make up for what is missing."

And there in is the problem. That's okay if all you are expecting is this one protocol, and you're happy to except corrupt messages (i.e. your use case & suggested code), but not when the library also tries to distinguish between many other protocols and confirm if the message is indeed the type it detected. etc. e.g. Your partial/corrupt message could be another protocol's message.

laserir commented 3 years ago

I understand. Yes, all codes are recognized correctly, but with greater distance/fog they get less likely read. And yes, in our case we are only expecting this specific protocol; didn't think about the need to decode multiple protocols. What about using the strict parameter for that cause? When false, omit the first compliance check (already implemented) and modify the second compliance check to add the zeros and accept not matching (minimum) number of bits?

NiKiZe commented 3 years ago

Could you provide a raw dump from those times with great distance and fog?

crankyoldgit commented 3 years ago

I understand. Yes, all codes are recognized correctly, but with greater distance/fog they get less likely read. And yes, in our case we are only expecting this specific protocol; didn't think about the need to decode multiple protocols. What about using the strict parameter for that cause? When false, omit the first compliance check (already implemented) and modify the second compliance check to add the zeros and accept not matching (minimum) number of bits?

That's kind of why I said this earlier:

I get why you may want that ability, and I'm happy to try to add that in a safe way. If you can supply a capture in the above scenario, we might be able to program/organise something for you.

If you can give us the data, I'll try to find a way you can metaphorically have your cake and eat it too. That is, I'll try to find a way or some switch you can flip in the library that gives you maximum decodability for this protocol and is safe for the rest of library's users.

Hence, we need some of your "poor quality" data examples to make sure what we add actually works and the library handles it accordingly for all use cases.

laserir commented 3 years ago

Thanks for your effort! It pretty much applies to all of the code samples. I think it's more the environment than the protocol. Under good conditions the library always works great.

NiKiZe commented 3 years ago

To improve the case of bad conditions, raw data from that condition is needed. (The received data)

laserir commented 3 years ago

Ah sorry!! Now I understand what you mean! Well, for that we‘ll have to wait. Arena is closed due to lockdown, at least until mid-january.

vitos1k commented 3 years ago

Will this protocol work for MilesTag2(open lasertag) protocol? http://hosting.cmalton.me.uk/chrism/lasertag/MT2Proto.pdf i'm interested in coding this proto, but i'm not familiar with github\pullrequests I can edit Library localy, and implement MilesTag2 support. Do i need to Fork library and make changes in my copy and then propose updates to parent library??

crankyoldgit commented 3 years ago

Will this protocol work for MilesTag2(open lasertag) protocol? http://hosting.cmalton.me.uk/chrism/lasertag/MT2Proto.pdf

I skimmed that document. The short answer is I'm pretty sure it's a very different protocol. So no. Our LASERTAG protocol is not the same as that protocol.

i'm interested in coding this proto, but i'm not familiar with github\pullrequests

Cool. There are plenty of free HowTo, tutorials, and web pages that can tell you how to work git hub and pull requests. Search and read away!

I can edit Library localy, and implement MilesTag2 support. Do i need to Fork library and make changes in my copy and then propose updates to parent library??

That's the best way to do it. When you've forked, got your updates working, and available on github, you can create a PR (pull request) from your fork to this one. It will show what all the changes are, and we can merge it in etc.

crankyoldgit commented 3 years ago

FYI, the some of the changes mentioned above have now been included in the new v2.7.14 release of the library.

laserir commented 3 years ago

Great! Will come back with raw data when the arena will be accessible again. As of today's news that might not be in January :-( Stay healthy everyone!

crankyoldgit commented 3 years ago

Friendly chase-up. Any luck?

laserir commented 3 years ago

Not yet. We're still under lockdown, at least until mid February, possibly even longer. :-/

crankyoldgit commented 3 years ago

No worries. I'll check in later in Feb. Good luck, stay safe etc. Get the vaccine if you can!

crankyoldgit commented 3 years ago

New Month, new ping. How goes the covid where you're at?

laserir commented 3 years ago

Germany... well, numbers are "okay" but still all leisure businesses forbidden. Lockdown until end of March... :-/ Sorry!

crankyoldgit commented 3 years ago

No worries. I'll chase up again in April then.

crankyoldgit commented 3 years ago

Hey, It's April. Just checking in. I'm assuming the usual covid scenario.

laserir commented 3 years ago

You bet...

crankyoldgit commented 3 years ago

No worries. I'll chase up again in April^H^H^H^H^HMay then.

crankyoldgit commented 3 years ago

Ping. Another month has passed. How goes the Covid-19 restrictions where you are? Try again in June?

laserir commented 3 years ago

Yep... Closed under "federal emergency break" until end of June, if not bankrupt by then.

crankyoldgit commented 3 years ago

Bugger, Oh well, next month then.

crankyoldgit commented 3 years ago

Ping. Another month has passed. How goes the Covid-19 restrictions where you are? Try again in July?

crankyoldgit commented 3 years ago

D'oh. I just re-read your earlier comment. "until end of June" ... double bugger.

NiKiZe commented 3 years ago

Any chance you have been allowed to start up?

crankyoldgit commented 3 years ago

@laserir Friendly ping to see how covid etc is going in your neck of the woods.

crankyoldgit commented 3 years ago

@laserir Another ping. I hope you are just busy with re-opening. 🤞 If we don't hear back from you soon (and that's a Metric soon, not an Imperial soon) I'll close this issue, and assume all is good.

laserir commented 3 years ago

Hi! Sorry to come back so late. So, after >1,5y of having no business, the place had to be closed permanently and is now being renewed and operated by a different company using a very much different System. All components are currently put in storage. If I get any chance to get my hands on them again, I will further investigate. Anyhow, we can agree that the current implementation works very well already and can be kept this way! Thanks for all your work and effort. It is very much appreciated! Stay safe and healthy!

crankyoldgit commented 3 years ago

I'm sorry the business went under. Thanks for letting us know etc. Good luck with your next opportunity.

P.s. Covid sucks!