Skip to content
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

Milestag2 #1380

Merged
merged 23 commits into from
Jan 16, 2021
Merged

Milestag2 #1380

merged 23 commits into from
Jan 16, 2021

Conversation

vitos1k
Copy link
Contributor

@vitos1k vitos1k commented Jan 10, 2021

I've implemented MilesTag2 Lasertag protocol described here:
http://hosting.cmalton.me.uk/chrism/lasertag/MT2Proto.pdf

decoding actuall meaning of bit's in protocol is on users side, they could just macro it for example:

#define MT_SHOT(player, team, damage) (unsigned long)(((player & 127) << 6) | ((team   & 3  ) << 4) | (damage & 15 ))
#define MT_PLAYER_VALUE(shot_data) (unsigned int)((shot_data >> 6) & 127)
#define MT_TEAM_VALUE(shot_data)   (unsigned short)((shot_data >> 4) & 3)
#define MT_DAMAGE_VALUE(shot_data) (unsigned short)(shot_data & 15)

To detect last bits in protocol, i've implemented functions with "space first, mark second" order, based on genericSend/matchGeneric

Tested it on ESP8266 and ESP32 and it works fine for me. But i don't have actual commercial Lasertag taggers and recievers with MilesTag2, to test against real world conditions. But i think this should be a good start.

X-Ref #1360

Implementing more obvious and direct SEND function, to trace some SPACE inconsistency.
- Bugfixing some intervals and found out that DutyCycle = 25-30% works great, even with my crappy vs1838b

- Implemented in decode GAP recognition in case of repeated message, but commented it out, because actual RAW GAP isn't 100ms as i set it, it actually is ~34ms, which is wierd, and i can't find out why
…ed from SonyProtocol, they were excessive. Fixed some assignements
…tion about REPEATs in MilesTag2 description, so they are arbitrary for now.
@crankyoldgit crankyoldgit self-requested a review January 11, 2021 05:57
@crankyoldgit crankyoldgit self-assigned this Jan 11, 2021
@crankyoldgit
Copy link
Owner

I'll have a proper look through the PR soon, but here are some automated reported linter issues so far:

$ python cpplint.py --extensions=c,cc,cpp,ino --headers=h,hpp {src,src/locale,test,tools}/*.{h,c,cc,cpp,hpp,ino} examples/*/*.{h,c,cc,cpp,hpp,ino}

src/IRrecv.h:514:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRsend.h:240:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.h:686:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRsend.h:687:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:9:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:9:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_MilesTag2.cpp:9:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_MilesTag2.cpp:17:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:38:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:39:  Missing space after ,  [whitespace/comma] [3]
src/ir_MilesTag2.cpp:44:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:45:  Missing space after ,  [whitespace/comma] [3]
src/ir_MilesTag2.cpp:53:  Redundant blank line at the start of a code block should be deleted.  [whitespace/blank_line] [2]
src/ir_MilesTag2.cpp:56:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_MilesTag2.cpp:60:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:65:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:66:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:99:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:104:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:114:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:117:  Missing space after ;  [whitespace/semicolon] [3]
src/ir_MilesTag2.cpp:119:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:120:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:122:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:123:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:125:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:128:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:133:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:142:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:664:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:718:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:906:  Missing space after ,  [whitespace/comma] [3]
src/IRsend.cpp:909:  Missing space after ,  [whitespace/comma] [3]
src/IRsend.cpp:911:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/IRsend.cpp:911:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 35

and:
You need to add your new protocol name to strToDecodeType() and/or typeToString() in IRutils.cpp.

[ RUN      ] TestUtils.TypeStringConversionRangeTests
IRutils_test.cpp:501: Failure
Expected equality of these values:
  i
    Which is: 98
  strToDecodeType(typeToString((decode_type_t)i).c_str())
    Which is: -1
Protocol  doesn't decode from a string correctly

and some how you've modified IRsend::defaultBits(decode_type_t::MIDEA24); to give the incorrect value.

[ RUN      ] TestUtils.Housekeeping
ir_Midea_test.cpp:957: Failure
Expected equality of these values:
  kMidea24Bits
    Which is: 24
  IRsend::defaultBits(decode_type_t::MIDEA24)
    Which is: 14
[  FAILED  ] TestUtils.Housekeeping (0 ms)

and add a "Suported Devices:" comment to your file.

The following files had no supports section:
	ir_MilesTag2

@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 11, 2021

I'll have a proper look through the PR soon, but here are some automated reported linter issues so far:

$ python cpplint.py --extensions=c,cc,cpp,ino --headers=h,hpp {src,src/locale,test,tools}/*.{h,c,cc,cpp,hpp,ino} examples/*/*.{h,c,cc,cpp,hpp,ino}

src/IRrecv.h:514:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRsend.h:240:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.h:686:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRsend.h:687:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:9:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:9:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_MilesTag2.cpp:9:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_MilesTag2.cpp:17:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:38:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:39:  Missing space after ,  [whitespace/comma] [3]
src/ir_MilesTag2.cpp:44:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:45:  Missing space after ,  [whitespace/comma] [3]
src/ir_MilesTag2.cpp:53:  Redundant blank line at the start of a code block should be deleted.  [whitespace/blank_line] [2]
src/ir_MilesTag2.cpp:56:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_MilesTag2.cpp:60:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:65:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:66:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:99:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:104:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:114:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:117:  Missing space after ;  [whitespace/semicolon] [3]
src/ir_MilesTag2.cpp:119:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:120:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:122:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:123:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_MilesTag2.cpp:125:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:128:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_MilesTag2.cpp:133:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/ir_MilesTag2.cpp:142:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:664:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:718:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRsend.cpp:906:  Missing space after ,  [whitespace/comma] [3]
src/IRsend.cpp:909:  Missing space after ,  [whitespace/comma] [3]
src/IRsend.cpp:911:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/IRsend.cpp:911:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 35

and:
You need to add your new protocol name to strToDecodeType() and/or typeToString() in IRutils.cpp.

[ RUN      ] TestUtils.TypeStringConversionRangeTests
IRutils_test.cpp:501: Failure
Expected equality of these values:
  i
    Which is: 98
  strToDecodeType(typeToString((decode_type_t)i).c_str())
    Which is: -1
Protocol  doesn't decode from a string correctly

and some how you've modified IRsend::defaultBits(decode_type_t::MIDEA24); to give the incorrect value.

[ RUN      ] TestUtils.Housekeeping
ir_Midea_test.cpp:957: Failure
Expected equality of these values:
  kMidea24Bits
    Which is: 24
  IRsend::defaultBits(decode_type_t::MIDEA24)
    Which is: 14
[  FAILED  ] TestUtils.Housekeeping (0 ms)

and add a "Suported Devices:" comment to your file.

The following files had no supports section:
	ir_MilesTag2

Addressed most of the issues, except the support comment. I don't have actual Hardware (lasertag taggers and recievers) on my hands right now. And due to lockdown, i can't test it. I've added this comment

// Supports:
//   Brand: Theoretically,  Model: MILESTAG2 SUPPORTED HARDWARE

@crankyoldgit
Copy link
Owner

Cool. Now that it's passing the automated checks, I'll try to get to reviewing it tomorrow.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for all the suggestions/corrections. You've done really well. Please don't see this as negative criticism. It's not meant to be.

I know you don't have access to any equipment etc at the moment, but do you have any recordings, dumps or other code that we can use in the interim to confirm/check we are producing/matching a correct signal?

Once we get this sorted a bit more, I'll add some basic unit tests to this PR, to at least make sure we are internally consistent. i.e. We can send something and decode it back. It may not work with real devices, but that's better than than nothing! ;-)

src/IRrecv.h Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Show resolved Hide resolved
src/ir_MilesTag2.cpp Outdated Show resolved Hide resolved
src/IRtext.cpp Outdated Show resolved Hide resolved
src/IRsend.h Outdated Show resolved Hide resolved
src/IRsend.cpp Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Outdated Show resolved Hide resolved
src/IRrecv.cpp Outdated Show resolved Hide resolved
Merging 2 functions and decode types into ONE Milestag2 as suggested by review
@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 13, 2021

Addressed all the issues. But while testing it via IRrecvDumpV2.ino i realized that Milestag2 protocol decoded as SONY protocol by library.
I send irsend.sendMilestag2(0x379);
i also tested irsend.sendSony38(0x379,14,0); and got similar Dump
The only trouble, SONY woudn't decode 24bits signal, in fact i'm surprised it even got decoded by SONY, since sony is working only with 12,15,20 bits messages.

 Timestamp : 000038.648
Library   : v2.7.13

Protocol  : SONY
Code      : 0x379 (14 Bits)
uint16_t rawData[29] = {2428, 612,  592, 592,  606, 602,  606, 600,  606, 600,  1210, 602,  1208, 602,  606, 602,  1208, 602,  1208, 600,  1210, 604,  1210, 602,  602, 604,  604, 604,  1214};  // SONY 379
uint64_t data = 0x379;


Timestamp : 000038.758
Library   : v2.7.13

Protocol  : SONY
Code      : 0x379 (14 Bits)
uint16_t rawData[29] = {2436, 606,  604, 602,  598, 608,  608, 598,  608, 598,  1210, 602,  1206, 604,  602, 604,  1212, 600,  1210, 600,  1210, 600,  1202, 610,  608, 600,  598, 584,  1236};  // SONY 379
uint64_t data = 0x379;

The only Issue i have now, is that ESP (irSend) is giving me bad timings right when i'm starting transmition. First ~5-8 bits (10-16 raw values) are OFF. And after that all timings become normal. I have to send it twice with short delay, and second time it goes alright.
Here is dump of invalid Data:

Timestamp : 000377.323
Library   : v2.7.13

Protocol  : UNKNOWN
Code      : 0xD1D773FF (15 Bits)
uint16_t rawData[29] = {2630, 374,  834, 370,  832, 372,  812, 404,  786, 422,  1254, 556,  1268, 550,  646, 564,  1204, 604,  1210, 596,  1204, 598,  1206, 596,  614, 592,  608, 596,  1204};  // UNKNOWN D1D773FF


Timestamp : 000379.377
Library   : v2.7.13

Protocol  : UNKNOWN
Code      : 0xD1D773FF (15 Bits)
uint16_t rawData[29] = {2630, 374,  834, 370,  832, 372,  808, 406,  782, 424,  1254, 554,  1268, 546,  650, 566,  1204, 604,  1210, 594,  1206, 598,  1202, 602,  616, 590,  612, 590,  1202};  // UNKNOWN D1D773FF

Delay is ~2seconds between two messages. See how header is 200us bigger than it should, and header space is 200-300us shorter than it should be. I'm excpecting {2400, 600, 600, 600, 600, 600, 600, 600, 600, 600, 1200, 600ish values}
See how it becomes more or less better to the end of the message.
I've tested this on Arduino UNO as reciever too, and got similar Dump.

My reciever is VS1838B, it's crappy, should i blame it?

@vitos1k vitos1k requested a review from crankyoldgit January 13, 2021 16:18
Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed all the issues. But while testing it via IRrecvDumpV2.ino i realized that Milestag2 protocol decoded as SONY protocol by library.
I send irsend.sendMilestag2(0x379);
i also tested irsend.sendSony38(0x379,14,0); and got similar Dump
The only trouble, SONY woudn't decode 24bits signal, in fact i'm surprised it even got decoded by SONY, since sony is working only with 12,15,20 bits messages.

Yeah, The code for decoding Sony is a tad of the "special" side. It was written ages ago, and to be as compact as possible, and match any/all of the Sony sizes (and then some) in an CPU efficient manor. i.e. It needs to be re-written.

You'll need to move the decodeMilestag2() etc code position in IRrecv::decode() to be before decodeSony() and add a note saying why. e.g. It's almost identical to SONY, but with different bit sizes. e.g. decodeSony() will match the 14 bit variant).

 Timestamp : 000038.648
Library   : v2.7.13

Protocol  : SONY
Code      : 0x379 (14 Bits)
uint16_t rawData[29] = {2428, 612,  592, 592,  606, 602,  606, 600,  606, 600,  1210, 602,  1208, 602,  606, 602,  1208, 602,  1208, 600,  1210, 604,  1210, 602,  602, 604,  604, 604,  1214};  // SONY 379
uint64_t data = 0x379;


Timestamp : 000038.758
Library   : v2.7.13

Protocol  : SONY
Code      : 0x379 (14 Bits)
uint16_t rawData[29] = {2436, 606,  604, 602,  598, 608,  608, 598,  608, 598,  1210, 602,  1206, 604,  602, 604,  1212, 600,  1210, 600,  1210, 600,  1202, 610,  608, 600,  598, 584,  1236};  // SONY 379
uint64_t data = 0x379;

What you've listed here is a Sony msg, and one of it's repeats. That's why they are identical.
e.g. Timestamp 000038.758 - 000038.648 = 0.110 seconds.

The only Issue i have now, is that ESP (irSend) is giving me bad timings right when i'm starting transmition. First ~5-8 bits (10-16 raw values) are OFF. And after that all timings become normal. I have to send it twice with short delay, and second time it goes alright.
Here is dump of invalid Data:

Timestamp : 000377.323
Library   : v2.7.13

Protocol  : UNKNOWN
Code      : 0xD1D773FF (15 Bits)
uint16_t rawData[29] = {2630, 374,  834, 370,  832, 372,  812, 404,  786, 422,  1254, 556,  1268, 550,  646, 564,  1204, 604,  1210, 596,  1204, 598,  1206, 596,  614, 592,  608, 596,  1204};  // UNKNOWN D1D773FF


Timestamp : 000379.377
Library   : v2.7.13

Protocol  : UNKNOWN
Code      : 0xD1D773FF (15 Bits)
uint16_t rawData[29] = {2630, 374,  834, 370,  832, 372,  808, 406,  782, 424,  1254, 554,  1268, 546,  650, 566,  1204, 604,  1210, 594,  1206, 598,  1202, 602,  616, 590,  612, 590,  1202};  // UNKNOWN D1D773FF

Delay is ~2seconds between two messages. See how header is 200us bigger than it should, and header space is 200-300us shorter than it should be. I'm excpecting {2400, 600, 600, 600, 600, 600, 600, 600, 600, 600, 1200, 600ish values}
See how it becomes more or less better to the end of the message.
I've tested this on Arduino UNO as reciever too, and got similar Dump.

My reciever is VS1838B, it's crappy, should i blame it?

That's definitely peculiar. The code as it is now shouldn't produce that. (I'll add some unit tests to confirm that later)

The VS1838B could be to blame. Not sure. Does sendSony() and sendSony38() give similar results? If they do, then it's probably the VS1838B, if not, then it's likely to be the sendMilestag2()` code.

src/IRrecv.h Outdated Show resolved Hide resolved
src/IRsend.h Outdated Show resolved Hide resolved
Comment on lines 19 to 20
// Constants are similar to Sony SIRC protocol, bassically they are very
// similar, just nbits are varying
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, Sony's protocol also uses a different modulation frequency (40kHz vs 38kHz):

const uint16_t kSonyStdFreq = 40000; // kHz

and a different duty cycle 33% vs 25%:
kSonyMinGap, kSonyRptLength, data, nbits, freq, true, repeat, 33);

and Sony requires 3 messages (2 repeats).
const uint16_t kSonyMinRepeat = 2;

i.e. They differ more than just bit lengths.

…nce this two a similar

minor readability changes
@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 14, 2021

resolved all issues with readability.

That's definitely peculiar. The code as it is now shouldn't produce that. (I'll add some unit tests to confirm that later)

The VS1838B could be to blame. Not sure. Does sendSony() and sendSony38() give similar results? If they do, then it's probably the VS1838B, if not, then it's likely to be the sendMilestag2()` code.

sendSony38() gives exact same behaviour. So i guess it's my recievers problem. It looks like it needs some time to warmup at the start of each transmission.

Comment on lines 28 to 29


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove two of these blank lines.

Comment on lines 61 to 74
/*
uint16_t gap_pos = 0;
// we got alot more data than we thought,
// let's find last GAP and work from it
if (results->rawlen >= (2 * nbits + 1))
{
for (uint16_t ind = 0 ; ind < results->rawlen; ind++)
{
if (matchAtLeast(*(results->rawbuf+ind),34000)) gap_pos = ind;
}
}

if (results->rawlen>gap_pos) offset = gap_pos+1;
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this commented out code.

@@ -0,0 +1,102 @@
// Copyright 2020 Victor Mukayev (vitos1k)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021 ;-)

#include "IRsend.h"
#include "IRutils.h"

// Constants
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add constants:

const uint16_t kMilesShotMask = 1 << (kMilesTag2ShotBits - 1);   // Shot packets have this bit as `0`
const uint32_t kMilesMsgMask = 1 << (kMilesTag2MsgBits - 1);   // Msg packets  have this bit as `1`
const uint8_t  kMilesMsgTerminator = 0xE8;

Per http://hosting.cmalton.me.uk/chrism/lasertag/MT2Proto.pdf Section 2.1 & 2.2

Comment on lines 78 to 79
case 14:
case 24:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case kMilesTag2ShotBits:
case kMilesTag2MsgBits:

Comment on lines 98 to 99
results->command = 0;
results->address = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove. (Because it has been moved.)

kMilesOneMark, kMilesSpace,
kMilesZeroMark, kMilesSpace,
0, kMilesRptLength, true)) return false;
// Success
Copy link
Owner

@crankyoldgit crankyoldgit Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add before // Success:

  results->command = 0;
  results->address = 0;

  // Compliance
  if (strict) {
    switch (nbits) {
      case kMilesTag2ShotBits:
        // Is it a valid shot packet?
        if (data & kMilesShotMask) return false;
        results->command = data & 0x3F;  // Team & Damage
        results->address = data >> 6;  // Player ID.
        break;
      case kMilesTag2MsgBits:
        // Is it a valid msg packet? i.e. Msg bit set & Terminator present.
        if (!(data & kMilesMsgMask) || ((data & 0xFF) != kMilesMsgTerminator))
          return false;
        results->command = (data >> 8) & 0xFF;  // Message data
        results->address = (data >> 16) & 0x7F;  // Message ID
        break;
    }
  }

Note: I haven't tested or compiled that code, so there could be errors.

* Unit tests
  - Housekeeping
  - Sending Shot & Msg sizes.
  - Self-decoding
  - Failure when packets are not formatted correctly.
* Rename constants/functions to be consistent in style.
* Restructure `decodeMilestag2()` to be cleaner/better.
* Minor optimisations.

Note: No real-world data as yet.
@crankyoldgit crankyoldgit added the Pending Confirmation Waiting for confirmation from user label Jan 15, 2021
@crankyoldgit
Copy link
Owner

@vitos1k Can you please git pull your repo from github. I've add several commits. It should work now. Please test.

@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 15, 2021

@vitos1k Can you please git pull your repo from github. I've add several commits. It should work now. Please test.

I've tested it and it still fails to detect miles tag. Through debugging i found out that in _matchGeneric() it fails to pass this row:

 // Check if there is enough capture buffer to possibly have the message.
  if (remaining < min_remaining) return 0;  // Nope, so abort.

i've added this printouts to find out:

// Check if there is enough capture buffer to possibly have the message.
  if (remaining < min_remaining) 
  {
    DPRINT("remaining:");
    DPRINT(remaining);
    DPRINT(" < min_remaining:");
    DPRINT(min_remaining);
    DPRINTLN("  return FAIL");
    return 0;
  }  // Nope, so abort.

and got this output in my terminal:

Attempting MilesTag2 decode
remaining:29 < min_remaining:50  return FAIL
remaining:29 < min_remaining:30  return FAIL
Attempting MilesTag2 decode
remaining:29 < min_remaining:50  return FAIL
remaining:29 < min_remaining:30  return FAIL

@crankyoldgit
Copy link
Owner

@vitos1k Can you please git pull your repo from github. I've add several commits. It should work now. Please test.

I've tested it and it still fails to detect miles tag. Through debugging i found out that in _matchGeneric() it fails to pass this row:

 // Check if there is enough capture buffer to possibly have the message.
  if (remaining < min_remaining) return 0;  // Nope, so abort.

i've added this printouts to find out:

// Check if there is enough capture buffer to possibly have the message.
  if (remaining < min_remaining) 
  {
    DPRINT("remaining:");
    DPRINT(remaining);
    DPRINT(" < min_remaining:");
    DPRINT(min_remaining);
    DPRINTLN("  return FAIL");
    return 0;
  }  // Nope, so abort.

and got this output in my terminal:

Attempting MilesTag2 decode
remaining:29 < min_remaining:50  return FAIL
remaining:29 < min_remaining:30  return FAIL
Attempting MilesTag2 decode
remaining:29 < min_remaining:50  return FAIL
remaining:29 < min_remaining:30  return FAIL

Got the raw message it's failing on so I can debug it?

@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 15, 2021

Got the raw message it's failing on so I can debug it?

sure

uint16_t rawData[29] = {2440, 602,  608, 600,  606, 600,  606, 600,  602, 606,  1208, 602,  1216, 596,  604, 600,  1214, 598,  1212, 600,  1208, 604,  1208, 602,  606, 600,  610, 596,  1210};  // MILESTAG2 379

btw if you call

irsend.sendMilestag2(0x0379);

It would produce the same raw data.

@crankyoldgit
Copy link
Owner

Got the raw message it's failing on so I can debug it?

sure

uint16_t rawData[29] = {2440, 602,  608, 600,  606, 600,  606, 600,  602, 606,  1208, 602,  1216, 596,  604, 600,  1214, 598,  1212, 600,  1208, 604,  1208, 602,  606, 600,  610, 596,  1210};  // MILESTAG2 379

btw if you call

irsend.sendMilestag2(0x0379);

It would produce the same raw data.

Thanks. I've probably got an off-by-one error in my new code.

@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 15, 2021

Got the raw message it's failing on so I can debug it?

sure

uint16_t rawData[29] = {2440, 602,  608, 600,  606, 600,  606, 600,  602, 606,  1208, 602,  1216, 596,  604, 600,  1214, 598,  1212, 600,  1208, 604,  1208, 602,  606, 600,  610, 596,  1210};  // MILESTAG2 379

btw if you call

irsend.sendMilestag2(0x0379);

It would produce the same raw data.

Thanks. I've probably got an off-by-one error in my new code.

looking through your matchData() function, I was wondering, isn't it dangerous to call function inside function itself?
Also if:
(MSBfirst == false) & (expectlastspace == false)
this would reverse bits twice, after matching all even bits, then adding one more bit, and then reversing it again, or am i missing something?

* Needed to subtract `1` from `min_remaining` in `_matchGeneric()` when not expecting a trailing space.
* Unit tests to cover those cases.
@crankyoldgit
Copy link
Owner

looking through your matchData() function, I was wondering, isn't it dangerous to call function inside function itself?

No, not if you know what you're doing. It's call recursion.

Also if:
(MSBfirst == false) & (expectlastspace == false)
this would reverse bits twice, after matching all even bits, then adding one more bit, and then reversing it again, or am i missing something?

I think you're missing something.

I've fixed the off-by-one error, and pushed the commit to your repo, so please git pull again and test it out.

It should now match your example case. Let me know how it goes please?

@vitos1k
Copy link
Contributor Author

vitos1k commented Jan 16, 2021

looking through your matchData() function, I was wondering, isn't it dangerous to call function inside function itself?

No, not if you know what you're doing. It's call recursion.

Also if:
(MSBfirst == false) & (expectlastspace == false)
this would reverse bits twice, after matching all even bits, then adding one more bit, and then reversing it again, or am i missing something?

I think you're missing something.

I've fixed the off-by-one error, and pushed the commit to your repo, so please git pull again and test it out.

It should now match your example case. Let me know how it goes please?

Yes! it works now as expected! Thanks!

@crankyoldgit crankyoldgit removed Pending Confirmation Waiting for confirmation from user more info labels Jan 16, 2021
@crankyoldgit crankyoldgit self-requested a review January 16, 2021 06:54
Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the confirmation. Looks good to me now.
I'll get @NiKiZe to also review this PR as I've contributed significantly too. i.e. To review my code ;-)

@crankyoldgit crankyoldgit requested a review from NiKiZe January 16, 2021 06:56
@crankyoldgit crankyoldgit dismissed their stale review January 16, 2021 07:01

I'm happy with it as is, just pending a review from someone else.

Copy link
Collaborator

@darshkpatel darshkpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crankyoldgit crankyoldgit merged commit 667ed87 into crankyoldgit:master Jan 16, 2021
@crankyoldgit
Copy link
Owner

Thanks @darshkpatel !

@vitos1k vitos1k deleted the milestag2 branch January 16, 2021 12:34
crankyoldgit added a commit that referenced this pull request Feb 12, 2021
_v2.7.15 (20210213)_

**[BREAKING CHANGES]**
- Some Daikin2 constants have been changed. (#1393)

**[Features]**
- Experimental basic support for EcoClim 56 & 15 bit protocols. (#1397 #1410)
- MITSUBISHI_AC: Add support for enabling Weekly Timer. (#1403 #1404)
- Mitsubishi ACs: Improve handling swing/vane settings. (#1399 #1401)
- MITSUBISHI_AC: Add support for half degrees. (#1398 #1400)
- Add `irutils::addSwing[V|H]ToString()` and adjust some constants (#1365 #1393)
- SharpAc: Add support for model A903, and improve `IRac` fan & power control. (#1387 #1390)
- Experimental support for Milestag2 (#1360 #1380)

**[Misc]**
- Improve `IRac::sendAc()` documentation. (#1408 #1409)
- refactor ir_Transcold (#1407)
- refactor ir_Toshiba (#1395)
- Fix Travis-CI build issues. (#1396)
- refactor ir_Teco (#1392)
- Fujitsu A/C: Add warning/suggestions for AR-RAH1U devices (#1376 #1386)
- refactor ir_Technibel (#1385)
- Add the new logo and banner 🎉 (#1371 #1372)
- Update references to sbprojects website. (#1381 #1383)
- refactor ir_Tcl (#1379)
@crankyoldgit crankyoldgit mentioned this pull request Feb 12, 2021
crankyoldgit added a commit that referenced this pull request Feb 13, 2021
_v2.7.15 (20210213)_

**[BREAKING CHANGES]**
- Some Daikin2 constants have been changed. (#1393)

**[Features]**
- Experimental basic support for EcoClim 56 & 15 bit protocols. (#1397 #1410)
- MITSUBISHI_AC: Add support for enabling Weekly Timer. (#1403 #1404)
- Mitsubishi ACs: Improve handling swing/vane settings. (#1399 #1401)
- MITSUBISHI_AC: Add support for half degrees. (#1398 #1400)
- Add `irutils::addSwing[V|H]ToString()` and adjust some constants (#1365 #1393)
- SharpAc: Add support for model A903, and improve `IRac` fan & power control. (#1387 #1390)
- Experimental support for Milestag2 (#1360 #1380)

**[Misc]**
- Improve `IRac::sendAc()` documentation. (#1408 #1409)
- refactor ir_Transcold (#1407)
- refactor ir_Toshiba (#1395)
- Fix Travis-CI build issues. (#1396)
- refactor ir_Teco (#1392)
- Fujitsu A/C: Add warning/suggestions for AR-RAH1U devices (#1376 #1386)
- refactor ir_Technibel (#1385)
- Add the new logo and banner 🎉 (#1371 #1372)
- Update references to sbprojects website. (#1381 #1383)
- refactor ir_Tcl (#1379)
crankyoldgit added a commit that referenced this pull request Feb 13, 2021
##_v2.7.15 (20210213)_

**[BREAKING CHANGES]**
- Some Daikin2 constants have been changed. (#1393)

**[Features]**
- Experimental basic support for EcoClim 56 & 15 bit protocols. (#1397 #1410)
- MITSUBISHI_AC: Add support for enabling Weekly Timer. (#1403 #1404)
- Mitsubishi ACs: Improve handling swing/vane settings. (#1399 #1401)
- MITSUBISHI_AC: Add support for half degrees. (#1398 #1400)
- Add `irutils::addSwing[V|H]ToString()` and adjust some constants (#1365 #1393)
- SharpAc: Add support for model A903, and improve `IRac` fan & power control. (#1387 #1390)
- Experimental support for Milestag2 (#1360 #1380)

**[Misc]**
- Improve `IRac::sendAc()` documentation. (#1408 #1409)
- refactor ir_Transcold (#1407)
- refactor ir_Toshiba (#1395)
- Fix Travis-CI build issues. (#1396)
- refactor ir_Teco (#1392)
- Fujitsu A/C: Add warning/suggestions for AR-RAH1U devices (#1376 #1386)
- refactor ir_Technibel (#1385)
- Add the new logo and banner 🎉 (#1371 #1372)
- Update references to sbprojects website. (#1381 #1383)
- refactor ir_Tcl (#1379)
@crankyoldgit
Copy link
Owner

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

@vitos1k
Copy link
Contributor Author

vitos1k commented Feb 13, 2021

That's great to hear! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants