Added transmit and receive support for devices using Holtek HT6P20X s…

[bau-sec]

…eries encoders via a new protocol. See the datasheet for the waveform. This protocol uses pulses that start low instead of high.

[bau-sec]

This is similar to PR #24. I see what @ricardojlrufino was trying to do and can explain if you have questions, but hopefully the comments I included clear things up.

If you look at page 6 of this datasheet, http://pdf.datasheetcatalog.com/datasheets/120/170613_DS.pdf, you can see the waveform I'm trying to receive and transmit. The big difference is that the pulses start low and then go high instead of starting high and going low.

[fingolfin]

Please don't use hungarian notation, it clearly does not match the surrounding code.

(Yes, I know it is used in other parts of rc-switch, which overall uses highly inconsistent coding styles; but my rule of thumb is that at least things which are inside the same function or struct should be consistent with each other :-)

[fingolfin]

Hmm, if you instead called this boolean invertHighAndLow, or just boolean invert, and then put the high value into the low field, and vice versa, wouldn't this avoid most of the changes in receiveProtocol? Personally, I'd find that easier to grasp, as that is really all that happens: the meanings of high and low are swapped.

Alas, talk is cheap, work is hard: You patch works (as you are using it), I am just hypothesizing... :-)

[bau-sec]

I agree that the code in my change could use some readability improvements. I thought about another alternative as well. What if we refactored the HighLow struct a bit:

struct Pulse {
    byte duration; //in pulse lengths as defined by a protocol
    uint8_t logicLevel; //HIGH or LOW
};

struct PulseSequence { //formally HighLow
     Pulse first;
     Pulse second;
};

struct Protocol {
    int pulseLength;
    PulseSequence syncFactor;
    PulseSequence zero;
    PulseSequence one;
};

I think this would be more readable than a boolean and remove a bunch of conditional code in my initial pull request.

What do you think? It's a bigger change than my initial PR, but it's more readable.

[bau-sec]

The latest commit to this PR is a stab at what I suggested in my previous comment. It's a bigger change, so I'm definitely open to feedback and any style tweaks that might be needed.

[fingolfin]

Yo, I think this is way overengineered now :-). You are solving problems which don't exist so far (there are no which arbitrarily switch between high-low and low-high ordering in pulses), at the cost of making the code bigger (= more RAM and flash is used up) and more complicated.

A simple "bool invertedSignal" in each protocol really should be enough. :-).

[fingolfin]

COUNT_OF is not a standard macro, so I wouldn't want to use it. What's wrong with numProto? The only argument against it I can think of is that it wastes 2 bytes of flash -- while I think the compiler will actually optimize it away anyway, we could make sure of that by turning it into an enum.

However, considering that this patch adds two bytes *per pulse pair, so 6 bytes per protocol, I don't think that's what you had in mind here... or is it? :-)

[bau-sec]

I rolled back the PulseSequence refactor as suggested and went with a tweaked version of the original commit.

[bau-sec]

@fingolfin Any issues remaining with this pull request?

[fingolfin]

Haven't had time to look at it yet, so can't really say.

[fingolfin]

(Note to self: we really should update https://github.com/sui77/rc-switch/wiki/KnowHow_LineCoding one of these days. And perhaps the other wiki pages, too; imho they are not that helpful right now :/ but as usual, writing good documentation is hard, and not many people like doing it)

[fingolfin]

@bau-sec The patch looks pretty good to me now, and I am sorry for leaving it in limbo so long (but I only have access to my testing hardware during weekends, and am loathe to merge stuff I could not try; unfortunately, the last weekends we were busy with other stuff :/).

If I didn't get a chance to test it by next weekend, I'll just merge it as is (after all, you tested the code yourself).

But thinking about it again, I now wonder: Is it really the best solution to have the "inversion" bit part of the protocol? Perhaps there should be a global switch, so one can use "inverted mode" with any protocol? And perhaps the receiveProtocol code should try both normal and inverted mode with all protocols?

(I am not asking you to change your PR, mind you, I am just thinking out loud -- haven't thought about it deeply yet, but I am guessing you have, and perhaps can point out the fatal drawbacks of that approach :-)

[bau-sec]

No worries on leaving the patch in limbo, I wouldn't want to merge anything without testing either.

As for making it a global switch, there are two considerations: receive and transmit.

For receive, a global switch might make sense for decoding an unknown protocol, but that's only because of the need interpret the sync signal in two different ways (with data starting at index 2 or with data starting at index 3). If #62 is implemented, the two different interpretations go away since the whole sync signal would be considered. After implementing #62, the receive code wouldn't care about the inversion at all. The interrupt is triggered on any change, the levels aren't considered.

For transmit I see little advantage to a global switch, though there might be something I'm not considering. To me if a transmission is occurring the caller should know what needs to be sent out as they're selecting a single protocol for transmission. A switch would suggest that the inverted protocol is the same, but it is a different waveform being generated which I would consider a different protocol. The only time a user might want this functionality is during the initial experimentation to find the proper settings for their device.

[bau-sec]

One other complication with receive: if the inversion is taken out of the protocol, there's no way for receive to differentiate between two hypothetical protocols that are inversions of each other. 0011 would look the same as 1100 to receive. Storing inversion in the protocol struct lets receive() know what logic levels it received. I don't think this was an intentional design decision, but something else to consider.