add DPT232.600 decoded (RGB) #42
Conversation
lib/decoder.js
Outdated
| } | ||
| } | ||
|
|
||
| if(len === 11) { |
There was a problem hiding this comment.
Are you sure that DPT232.600 is the only datatype that could have this package length?
There was a problem hiding this comment.
No, there are other DPTs with 3 bytes payload (which should also result in len = 11): DPT10 and DPT (date, time), but they haven't been implemented yet, so there is no conflict right now. To be perfectly honest, it is impossible to tell the DPT by looking at the length, even with some additional validation.
There was a problem hiding this comment.
Its a assumption made for DPT1,DPT5 and DPT9. Somehow I think that we don't have any conflicts for these type.
I don't feel comfortable to merge this, because if guys try to use other DPTs its kind of broken for them. I think its better to give the decision to the user for these types. I understand your argument that there are no other decode methods yet. I guess it would be only a matter of time we get the first issue.
I want the opinions from past committers about this. Maybe we get some replies.
@stasee @snowdd1 @marcopiraccini @anlumo
There was a problem hiding this comment.
I would not add assumptions for other DPT types, if there is no unique identification.
With my changes (you just merged), you get the raw data with the UNKN Data Type.
In my projects, I did the following:
I am using a unique naming convention for the group addresses in ETS. For example I use "ea301/1 Light Kitchen" for on / off (ein/aus) in the room number 301, Light 1, Room Kitchen.
Or "tmp301 Kitchen" for the room temperature in the room 301 (Kitchen).
I export the group addresses in a csv.
In my node projects I read the group address csv.
When receiving a read or write request, I do a lookup in my group address list and use the datatype that is identified by the name prefix.
There was a problem hiding this comment.
That's also what other implementations are doing, having a lookup table rather than "guessing", but I saw the length-based approach, so I implemented it.
The problem I have with the current implementation is the conversion to string:
if(type === 'UNKN') {
value = data.toString();
}
If we could output this in hex, one could also work with non-printable characters.
There was a problem hiding this comment.
Very good, it was still in place when I looked at things.
Just reject my PR then, I'll work based on the UNKN data.
There was a problem hiding this comment.
No, no.. we want your decodeDPT232 method. Please remove the assumption.
3 participants
This PR is based from thereapman/node-eibd, once you commit his PR this should fit in easily.
Sorry for the lack of test, hope you can commit it anyway.