Add DPT-en/decoding to libeibclient about knxd HOT 16 OPEN

I am working on this. You can see my progress here https://github.com/Makki1/knxd/tree/dpt-de/encode
I need to finish the unit-tests and this should be done.

from knxd.

Really cool/good news!

I'd be be happy to assist in testing, there are some traps with i.e. DPT9, the remainder is "just" to implement it.

from knxd.

I want to give you a short explanation how I thought about the de-encoding.

int KNX_Decode_Value(uint8_t *payload, int payload_length, KNXDatatype datatype, KNXValue *value);

uint8_t* payload... the raw dpt payload
int payload_length ... the length of the payload (to ensure not accessing an invald memory)
KNXDatatype datatype ... the knx dpt
KNXValue *value ... the decoded knx value 

typedef struct {
    uint8_t bValue;
    uint8_t cValue;
    uint16_t sValue;
    uint32_t iValue;
    uint64_t uiValue;
    double dValue;
    char *strValue;
    struct tm tValue;
} KNXValue;

typedef struct {
    unsigned short mainGroup; //DPT MainGroup 
    unsigned short subGroup; //SubGroup
    unsigned short index; //Index
} KNXDatatype;

Example:
DPT 11.001
Main Group = 11
SubGroup = 1
Index 0 = day
Index 1 = month
Index 2 = year

In case of DPT 11.001 the index will be ignored. The KNXValue.tValue will contain the Date.

DPT 2.001
Index 0 = v
Index 1 = c

In case of DPT 2.001-0 {MainGroup.SubGroup-Index} you will recieve the v bit (KNXValue.cValue, KNXValue.uiValue, KNXValue.iValue, KNXValue.bValue). DPT 2.001-1 will set the c bit of the package into the KNXValue pointer.

What do you think about that? We use this addressing schema in our company for our project. Therefore you don't need a lot of different structs.

In order to encode the KNXValue to the raw dpt value, you need to know the length of the dpt.

int KNX_Encode_Value(KNXValue *value, uint8_t *payload, int payload_length, KNXDatatype datatype);

KNXValue *value ... the decoded KNXValue
uint8_t *payload ... the raw payload
int payload_length ... length of the payload pointer
KNXDatatype ... target datatype

from knxd.

Looks fine, but I thought a step further:
(as it won't save much effort on the clients this way)
Step/Option 1: The client must not be aware of "payload sizes" in any way, only the GA and the DPT
Step/Option 2: The client must not even know the DPT, because it's configured (&read&written) inside knxd

from knxd.

Depends on what you want to do with the client. As you can see a DPT can be pretty all. You have different units, types, etc.
What can a client do with this information when it doesn't even know what it is?

I think the configuration part should be on the client side.

from knxd.

Looks fine. One question comes to my mind.
typedef struct {
uint8_t bValue;
uint8_t cValue;
uint16_t sValue;
uint32_t iValue;
uint64_t uiValue;
double dValue;
char *strValue;
struct tm tValue;
} KNXValue;

Why is this a struct? Isn't a union a better way?

from knxd.

I think the configuration part should be on the client side.

But then we can keep it like it is(?): fully on the client-side.
It IMHO makes no sense, if we enhance the API while the client still needs to know "payload", DPT and how to handle it in each case.
Correct me if I'm wrong, but thats no progress in making libeibcleint easier to use in any way(?)

from knxd.

In addition, EIS4/DPT11 is most challenging, (or weird) as it has to be combined to DPT10+DPT11 to make a useful timestamp in both directions - but for the remainder it is rather simple

from knxd.

@do13 you're right.

@Makki1 I don't it's getting much more complicated. I use the following function to get the APDU

int EIBGetGroup_Src (EIBConnection * con, int maxlen, uint8_t * buf, eibaddr_t * src, eibaddr_t * dest);

I can pass the buffer of these function to

int KNX_APDU_To_Payload(uint8_t *apdu, int apdu_len, uint8_t *payload, int payload_max_len);

And I receive just the payload. After this I can pass the payload to the decode function.

I don't know how you wanna do a configuration.

About DPT10+11, this are just implementation details.

from knxd.

I have not look at it in detail. But we have to work on a better Interface.

Some points:

• Why do you use strings.h? This is a non standard not portable header. I think you do not even use the functions. Remove it.
• Tests should be compiled on request only. Please add an configure option for that. This should be done independent from this issue.
• Allocating memory without clearly stating when to free is bad. A more object oriented solution is better here. Perhaps write it in c++ and just providing a c interface is better? Btw in your testing code you just did it: You do not free the allocated memory.

from knxd.

@timowi I fixed points 1&2. But where do I not free allocated memory? All the memory I use is on the stack.

from knxd.

But where do I not free allocated memory

Sorry, I though I did see the testing of ValueToString there. But it's not implemented yet.

Such allocations should have no impact outside the implementation. For example you can implement it object oriented in c by supplying create_KNXValue() and destroy_KNXValue(). In create_... set strValue to NULL, check before every allocation, free in destroy if not NULL. So the user of the api does not have to know about implementation.

As String is limited to 14 characters here the simplest (and safest) way is to just fixed char strValue[15]

From your malloc(size); strcpy(...) construct I assume you never saw strdup(). Have a look at it (man strdup, for example) this does exactly this.

For me your macros makes the code difficult to read. Especially having a return in there. Please consider removing at least some of them.

from knxd.

Well, it should be clarified first what language this function should be. C or C++?
If it is C++ I wouldn't agree with that interface. E.g. memory management was already stated - C++ can do much better.

And I think we should also set our mind on a different point fist: does it need to be a unified translation as proposed, or should be have different functions, one per DPT?

Especially due to the problem of having different C or C++ types that the KNX package will be mapped to - and the following program must handle it type safe as well - I would stick to different functions.
That would make future extensions more easy, as well as keep the result of a DPT like datetime more intuitive for the library user.

from knxd.

Well, it should be clarified first what language this function should be. C or C++?

I think it should stay simple - and most other client-APIs are "auto-built" -> so C

from knxd.

I looked a bit at the current code. I think this is completely the wrong approach. It does not fit in the autogenerated api at all. So we can completely keep it apart. This way it only work with c bindings and have to be written for every supported language.

For now I suggest moving this code to contrib/ as a datapoint de-/encoding library. (libknxdtp?) Then we should plan how to integrate with the current client concept.

Please add at least some comments. It's hard to find out how your api works. I stumbled a bit about your KNXDatatype.index implementation: If I got this right I need to do multiple call to KNX_APDU_To_Payload/KNX_Decode_Value with different index values to do the conversation?

KNXDatatype kd;
kd.index=0;
KNX_Decode_Value(payload, payloadl, DATATYPE_BINARY_CONTROL, kd);
// use kd
kd.index=1;
KNX_Decode_Value(payload, payloadl, DATATYPE_BINARY_CONTROL, kd);
// use kd

like the example? just to get the data? Sorry for the hard words, but this one of the worst apis I can think of. This way the client has to know everything about datapoints.

It should be as simple as

knxd_write_string(groupaddress, KNXDATATYPE_VARSTRING, string);
knxd_write_2bool(groupaddress, DATATYPE_BINARY_CONTROL, ctrl, state);
...

I think the right way is to do this on server side and add new communication packets to the client library. The code in the client library should be auto generated. We first need to plan the new packets. Something like writetogroupdpt/listengroupdpt. Parameter list should be variable depending on dpt. The server side should be implemented in an own c++ class.

@Makki1

Step/Option 1: The client must not be aware of "payload sizes" in any way, only the GA and the DPT
Step/Option 2: The client must not even know the DPT, because it's configured (&read&written) inside knx

I thin option 2 does not work. The client have to now about the DPT. The client creates or interprets the value so there must be knowledge about the DPT. But payload/payload_size is nothing the client have to worry about...

from knxd.

I agree with Timo, if we implement this, things should get more simple, not more complex.
We have the choice already to use libeibclient with knowing DPT, payload etc..
Though this is very error-prone, mostly with the (IEEE) float DPT's, the goal would be to avoid that by having a simple API in libeibclient.

from knxd.