RafaelKa / node-serialport-enocean-parser

ESP3 parser for nodes serialport
Do What The F*ck You Want To Public License
6 stars 2 forks source link

Feature - add getRawBuffer() method to ESP3Packet #3

Closed RafaelKa closed 8 years ago

RafaelKa commented 8 years ago

this change adds new method getRawBuffer() to the ESP3Packet

fixes #1

RafaelKa commented 8 years ago

@Holger-Will please check this out

Holger-Will commented 8 years ago

this solution sounds great! I'll have a go and test it...

Holger-Will commented 8 years ago

The current impl. throws for me with:

"list" argument must be an Array of Buffers

I'm on node 6.4.0. Its not possible to concat integers to a buffer. i have to go with something like this:

this.getRawBuffer = function () {
    return Buffer.concat(
        [
            new Buffer([this.syncByte]),
            new Buffer([0,this.header.dataLength]),
            new Buffer([this.header.optionalLength]),
            new Buffer([this.header.packetType]),
            new Buffer([this.crc8Header]),
            this.data,
            this.optionalData,
            new Buffer([this.crc8Data])
        ],
        8 + this.header.dataLength + this.header.optionalLength
    );
}

the problem with this is still, that dataLength should be encoded with 2 Bytes... in the above code i assume that dataLength is allways < 256 wich might not be the case... my current solution is to go with something like this

this.getRawBuffer = function () {
            var dl = new Buffer(2)
            dl.writeUInt16BE(0x05,0)
    return Buffer.concat(
        [
            new Buffer([this.syncByte]),
            dl,
            new Buffer([this.header.optionalLength]),
            new Buffer([this.header.packetType]),
            new Buffer([this.crc8Header]),
            this.data,
            this.optionalData,
            new Buffer([this.crc8Data])
        ],
        8 + this.header.dataLength + this.header.optionalLength
    );
}
Holger-Will commented 8 years ago

technical rant

On the other hand i would much prefer if the parser just returned the buffer as is. this would be most convenient and the packets would have the smallest footprint. you could have different Implementations which consume the raw buffer and parse it to whatever is currently needed.

For example, i have my telegram.js implementation wich turns the buffer into an internal representation which is best suited for what i need.

You turn the received bytes into a format which is best suited for your needs.

The most simple representation and common denominator of both is just the Buffer.

practicaly what happens currently is that we receive some bytes turn them into some js representation, turn them back to bytes to turn them into another representation... In the end an ESPPacket is just Bytes. Thats what the parser should return IMHO.

Holger-Will commented 8 years ago

please don't get me wrong here. I will be more than happy with the getRawBuffer() solution...

RafaelKa commented 8 years ago

@Holger-Will could you please provide a couple of different valid telegramms(with filled optional data meaybe). So i can draw up some tests...

Holger-Will commented 8 years ago

55000a0701eba5c87f710fffdba5e40001ffffffff47000d (4BS, a5) 55000707017ad509ffdba5ed0001ffffffff470096 (1BS, d5) 55000c070196d24000b00a010001a03d790001ffffffff5b0033 (VLD, d2) 55000707017af600ffd9b7812001ffffffff460050 (RPS, f6) 55000a0701eba540300287ffd9b7e50001ffffffff440016 (4BS Teach In, a5)

please also have a look at the tests in node-enocean. i'm generating a couple of telegrams for each supported eep...

RafaelKa commented 8 years ago

thanks I'll take a look on that.

Holger-Will commented 8 years ago

looks great!

good stuff with the testing... how about writing test for failures as well, like wrong crc or telegrams that are not fully transmittet? for example: what should happen if a packet is partly transmited and imediatly followed by a complete telegram? 55000a0701eba54055000707017af600ffd9b7812001ffffffff460050

RafaelKa commented 8 years ago

@Holger-Will so can i merge and release 0.0.3?

Holger-Will commented 8 years ago

Yes, please go ahead. But maybe consider publishing as 0.1.0 as per http://semver.org

Holger-Will commented 8 years ago

fanatastic! thanks for your valuable support!

RafaelKa commented 8 years ago

I played around with this and did something avesome. I'll provide by free time in future new api and deprecate this. Idea is, to use packet buffer instead of setting Properties and read from them all the properties for ESP3Packet.

RafaelKa commented 8 years ago

And following scenario is currently not implemented:

55[some <6Byte mishmash]55000707017ad509ffdba5ed0001ffffffff470096

packet after mishmash is lost! This will be solved in the next releases

Holger-Will commented 8 years ago

Cool, looking forward to it :-)