intel / ledmon

Enclosure LED Utilities
GNU General Public License v2.0
72 stars 44 forks source link

libled: Add more API details #160

Closed tasleson closed 10 months ago

tasleson commented 10 months ago

Clarify that the "path" input variable used in led_set and led_is_management_supported need to be used with output from the function led_device_name_lookup.

We may want to consider moving this functionality internally to the library, so that the API caller is not required to make this call first. The only downside would be some additional overhead which would be negligible.

mtkaczyk commented 10 months ago

Clarify that the "path" input variable used in led_set and led_is_management_supported need to be used with output from the function led_device_name_lookup.

The true is that it may not be a result of led_device_name_lookup if you know what you are doing. it is just a kind of "realpath", so it ensures you that path is correct but it is skippable if you know exactly what form is supported. I'm fine with documenting it but I would like to make softer requirement "this should be the result of led_device_name_lookup"

We may want to consider moving this functionality internally to the library, so that the API caller is not required to make this call first. The only downside would be some additional overhead which would be negligible.

The one advantage of current solution I see is that the user may lookup the devices one time and then use saved values. I think that we can maintain both solutions.

If you would like to have it build-in in led_set, we can make it configurable, by flag bool lookup_device (or mask, to be ready for more settings like this?):

led_status_t led_set(struct led_ctx *ctx, const char *path, enum led_ibpi_pattern ibpi, bool lookup_device)

Even if overhead is negligible, in general API will be used to call led_set so I would like to avoid this overhead. We will need to additionally allocate some memory which may not be needed in some cases. What do you think?

tasleson commented 10 months ago

The true is that it may not be a result of led_device_name_lookup if you know what you are doing. it is just a kind of "realpath", so it ensures you that path is correct but it is skippable if you know exactly what form is supported. I'm fine with documenting it but I would like to make softer requirement "this should be the result of led_device_name_lookup"

Updated

Even if overhead is negligible, in general API will be used to call led_set so I would like to avoid this overhead. We will need to additionally allocate some memory which may not be needed in some cases. What do you think?

Let's just leave it as is. It will be very apparent to any API user that things aren't correct if they neglect to call the lookup first.