Add device label parameter

[arneboe]

This adds the device_label parameter.
This PR sits on top of #92 and thus the diff contains a lot of changes that have nothing to do with this PR. The last commit contains the actual content of this PR.

[someweisguy]

Thanks again!

I am thinking that we will also need to add a getter/setter for device label. RDM_PID_DEVICE_LABEL is an NVS parameter so the setter must also write to NVS. And getters must return values by copy, not by reference. So the getter should call strncpy() or rdm_pd_emplace() to copy the string into a user's buffer.

I'm happy to add this feature myself in a future PR if that is helpful!

[arneboe]

I am thinking that we will also need to add a getter/setter for device label. RDM_PID_DEVICE_LABEL is an NVS parameter so the setter must also write to NVS. And getters must return values by copy, not by reference. So the getter should call strncpy() or rdm_pd_emplace() to copy the string into a user's buffer.

I'm happy to add this feature myself in a future PR if that is helpful!

I have not yet looked into the whole nvs thing. I'll take a look and implement it :)

Thinking about the NVS, why are only some parameters stored in the nvs?
From a users point of view: If I configure a fixture I expect all parameters to survive a reboot.

[arneboe]

And getters must return values by copy, not by reference. So the getter should call strncpy() or rdm_pd_emplace() to copy the string into a user's buffer.

I implemented the getter but copying the string feels strange and error prone.

What is the reason for getters returning by copy?
Especially in the case of device_label it would be much easier to just return the const char*. The string is stored on the heap anyway and the pointer will remain valid forever because there is no way to remove parameters.

[someweisguy]

Thinking about the NVS, why are only some parameters stored in the nvs?
From a users point of view: If I configure a fixture I expect all parameters to survive a reboot.

Excellent question! A lot of PIDs are essentially constants that depend on your code. Therefore, they will survive a reboot just by the virtue of being a constant. Take, for example, RDM_PID_SOFTWARE_VERSION_LABEL. This PID is defined in dmx_config_t so on reboots, it will always be the same. In other words, you are correct in assuming that all parameters will survive a reboot. It's just that some PIDs don't need to be placed in NVS to survive.

The current exception is RDM_PID_IDENTIFY_DEVICE. Now that we are discussing this, I see how this could be a mistake. I can add a patch which ensures that the identify device state survives reboots.

I may also remove the NVS column from the readme docs. I see now that it may be confusing for readers. It is also not currently correct because it does not have an NVS checkmark for RDM_PID_DEVICE_LABEL.

[someweisguy]

What is the reason for getters returning by copy?
Especially in the case of device_label it would be much easier to just return the const char*. The string is stored on the heap anyway and the pointer will remain valid forever because there is no way to remove parameters.

I am having a hard time remembering exactly why I went with return by copy. I think it was a thread-safety issue. If you passed these variables around by their pointers, you could be in the middle of printing the device label to the terminal when the task is pre-empted by a RDM request which updates the device_label to a new value. This would result in the terminal printing part of the pre-update device_label and part of the post-update device_label. In other words, garbage data.

Understanding thread-safety is difficult for me so if I am incorrect in my reasoning here, please let me know! :)

[arneboe]

I am having a hard time remembering exactly why I went with return by copy. I think it was a thread-safety issue. If you passed these variables around by their pointers, you could be in the middle of printing the device label to the terminal when the task is pre-empted by a RDM request which updates the device_label to a new value. This would result in the terminal printing part of the pre-update device_label and part of the post-update device_label. In other words, garbage data.

Understanding thread-safety is difficult for me so if I am incorrect in my reasoning here, please let me know! :)

Ahh, I haven't thought about multi-threading. I wasn't aware that the api should be thread-safe.
In that case, the getter and setter have to use the same lock that dmx_receive() uses while copying the data.
Since copying such a small string is fast, that should not be a problem. Just makes the api a little bit harder to use because the user has to provide a pre-allocated buffer.

In general I am not sure if implementing thread-safety is worth the effort to be honest.
All user accessible methods have to lock and copy data. That is a lot of additional implementation detail.
E.g. rdm_pd_find() would have to copy as well (which it currently doesn't).

Users that are accessing data from the same task do not benefit from the locking and copying. It just makes things slower and the api harder to work with.

Users that are accessing data from a different task usually know that they are doing so and can guard against corrupt data by using a mutex around the dmx_receive() call and do their own copying. This only works as long as the interrupts do not modify data that is accessible by the user.

I have never seen an embedded api that deals with thread-safety internally. Usually that is left to the user.
(But I have only worked with a small number of embedded libs, so this might just be bias :D)

[someweisguy]

I have never seen an embedded api that deals with thread-safety internally. Usually that is left to the user.

To be honest, I don't have much experience with what an embedded project should look like. If it is true that thread-safety should have been left to the user, then that is my mistake. :)

E.g. rdm_pd_find() would have to copy as well (which it currently doesn't).

rdm_pd_find() is used within the library to find the parameter data. At some point there needs to be a function that returns by pointer. This function does not return a const pointer. This is done so that parameters in the RDM responder can be updated.

In general I am not sure if implementing thread-safety is worth the effort to be honest.
All user accessible methods have to lock and copy data. That is a lot of additional implementation detail.

That is fair. I think you're correct that copy-by-pointer can offer significant advantages. I'll have to think about this. If there aren't any major issues, perhaps it could be part of the v4 API?

Edit: I should add that I had a difficult time coupling the DMX code of this library to the RDM code. I think, in general, the DMX code should be thread-safe. I spent a considerable amount of time trying to make the RDM API match the DMX API, but maybe I overdid it. I may do some experiments in the dev branch.

[arneboe]

rdm_pd_find() is used within the library to find the parameter data. At some point there needs to be a function that returns by pointer. This function does not return a const pointer. This is done so that parameters in the RDM responder can be updated.

Opps, my bad. you are right :)

In general I think that I have lost track of what to do with this pr now :D What do I have to do to get them merged? :)

[someweisguy]

Last thing is to change dmx_get_device_label(). Could you rename this to rdm_get_device_label() and move it to the src/rdm/parameters.c and parameters.h files? Let me know if you have any questions!

[arneboe]

Last thing is to change dmx_get_device_label(). Could you rename this to rdm_get_device_label() and move it to the src/rdm/parameters.c and parameters.h files? Let me know if you have any questions!

Done, sorry it took so long :-)