bluerobotics / ping-cpp

C++ API implementation of ping-protocol
13 stars 13 forks source link

feat: add ping360 generations #36

Closed incebellipipo closed 2 years ago

incebellipipo commented 2 years ago

With this pull request, ping360 headers will also be generated.

incebellipipo commented 2 years ago

@patrickelectric

Those lines don't produce the correct output. device_id_data is not declared inside the generated header file. Also, method name is device_id , not set_device_id. Although I'm not so sure what set_device_id does.

bool Ping360::device_id(uint8_t _id, uint8_t _reserved, bool verify)
{
    ping360_device_id message;
    message.set_id(_id);
    message.set_reserved(_reserved);
    writeMessage(message);
    // Check if we have a reply from the device
    if (!request(Ping360Id::DEVICE_ID)) {
        return false;
    }
    // Read back the data and check that changes have been applied
    if (verify
        && (device_id_data.id != _id
        || device_id_data.reserved != _reserved)) {
        return false;
    }
    return true;
}

https://github.com/bluerobotics/ping-protocol/blob/34db8b1880628fe1abd7eb5df132e6c7dd0d15cd/src/definitions/ping360.json#L3-L20

incebellipipo commented 2 years ago

There is an inconsistency between those two definitions. Nonetheless, it should generate the correct header. If I can fix the issue, it will be in the same pull request again. I guess, travis would do the last check for us.

https://github.com/bluerobotics/ping-protocol/blob/34db8b1880628fe1abd7eb5df132e6c7dd0d15cd/src/definitions/ping360.json#L3-L20

https://github.com/bluerobotics/ping-protocol/blob/34db8b1880628fe1abd7eb5df132e6c7dd0d15cd/src/definitions/ping1d.json#L3-L14

incebellipipo commented 2 years ago

Here is my conclusion,

It became a breaking change now. It can be done with supervision or discussion from the authors of the repository.

set_device_id method and description of the object within ping-protocol has some sort of conflict that creates a problem in the code generation process. https://github.com/GSO-soslab/ping-cpp/blob/b77b8cdfedf8bc5944c29e5c93257dbdc3ed0076/src/generate/templates/ping-device-ping360.cpp.in#L88 The fast workaround is to remove device_id from ping-protocol for Ping360. That is a big change. Considering that it is flagged as deprecated in the Ping1D definition file. https://github.com/GSO-soslab/ping-protocol/blob/1ec4d82467da8e98ad7d491a14bc922a30194dad/src/definitions/ping1d.json#L128

For those reasons, I'll leave that PR as draft.

patrickelectric commented 2 years ago

@incebellipipo take a look in #38, it applies some corrections around your patch.