[spec] led::trigger attribute

[ddemidov]

There is an issue with the current specification of led::trigger and led::triggers attributes. These differ from the rest of attributes/attribute sets (like motor::command and motor::commands) in the sense that single sysfs file is used to expose both possible values of the trigger as well as the current value:

$ cat /sys/class/leds/ev3-right0\:red\:ev3dev/trigger 
[none] mmc0 timer heartbeat default-on transient legoev3-battery-charging-or-full legoev3-battery-charging legoev3-battery-full legoev3-battery-charging-blink-full-solid rfkill0
$ echo timer > /sys/class/leds/ev3-right0\:red\:ev3dev/trigger

compare this with

$ cat /sys/class/tacho-motor/motor0/commands
run-forever run-to-abs-pos run-to-rel-pos run-timed run-direct stop reset
$ echo run-forever > /sys/class/tacho-motor/motor0/command

The cpp API does provide a way to access such attributes, but there is currently no way this can be described in the spec.json file.

We could introduce another attribute type in the specification for such attributes (I think led::trigger is the only example so far), something like string selector, and then update the templates for each of the bindings. What do you think?

[WasabiFan]

How are you thinking of specifying it? If the idea is to auto-generate both the getter for the whole list as well as the currently selected one, we'll need to define them separately with two different types: a "string selector list" and a "string selector current item", right?

[ddemidov]

I was thinking of changing the system name of Triggers attribute to trigger here for the setter to work. So the user-friendly name is still triggers, but it is populated by reading the trigger attribute. This would work at least in C++ as is. And the getter would know what to do because of the updated attribute type.

[ddemidov]

Sorry, I mixed up setters/getters in the previous post. The Triggers is read-only attribute, so it only has a getter. And the Trigger attribute is read/write, but I don't see any problem in implementing it if we have a special type for it. For example, the following patch worked for me before I remembered that these fragments should be autogenerated:

diff --git a/cpp/ev3dev.h b/cpp/ev3dev.h
index f7059cb..6890fc5 100644
--- a/cpp/ev3dev.h
+++ b/cpp/ev3dev.h
@@ -1226,7 +1226,7 @@ public:

   // Triggers: read-only
   // Returns a list of available triggers.
-  mode_set triggers() const { return get_attr_set("triggers"); }
+  mode_set triggers() const { return get_attr_set("trigger"); }

   // Trigger: read/write
   // Sets the led trigger. A trigger
@@ -1243,7 +1243,7 @@ public:
   // You can change the brightness value of a LED independently of the timer
   // trigger. However, if you set the brightness value to 0 it will
   // also disable the `timer` trigger.
-  std::string trigger() const { return get_attr_string("trigger"); }
+  std::string trigger() const { return get_attr_from_set("trigger"); }
   auto set_trigger(std::string v) -> decltype(*this) {
     set_attr_string("trigger", v);
     return *this;

[WasabiFan]

I don't quite understand...

The LED class looks like this in the file system:

$ ls /sys/class/leds/ev3-left1\:green\:ev3dev/
brightness  device  max_brightness  power  subsystem  trigger  uevent

So, to get the list of all available triggers, you read from trigger, split by space, and then remove the brackets that enclose the currently selected one. To get the currently-chosen trigger, you read from trigger, split by space, and find the item enclosed in brackets.

In our API, we want to generate a "triggers" property, which returns the list of all available triggers, and a "trigger" property, which gets (or sets) the currently-selected item. So, in our spec.json file, I'd imagine that we use a special type to denote "list all options" and a different type to denote "get current option".

Is my understanding the same as yours? If not, what am I misunderstanding?

[ddemidov]

I think we are talking about the same thing. The relevant part of spec.json could look like this:

{ "name": "Triggers", "systemName": "trigger", "type": "string array", "readAccess": true, "writeAccess": false},
{ "name": "Trigger", "systemName": "trigger", "type": "string selector", "readAccess": true, "writeAccess": true},

The type of the Trigger property should be something new, since the standard value getters won't be able to handle it. The type of the Triggers though could stay the same, if we are willing to put the square brackets handling functionality into the standard array getter (as its done in C++ currently). Or, if we want to introduce another array getter, we could invent some new type for Triggers as well.

Another option is to move the attributes out of direct attribute mappings into special properties section and then deal with them manually.

[WasabiFan]

OK, looks like we were using different language to explain the same concept. Yes, that looks good -- I think it makes sense to do it as you showed in the spec.json fragment.

This reminds me of the fact that I don't think we explain anywhere how the various types should be handled (e.g. How do you parse an array?). We should probably add that to the spec, or maybe add a document in the autogen folder about it. Then we can clearly explain the "selector" behavior.

[ddemidov]

Yes, that looks good

Ok, I'll do a PR for you to review.

This reminds me of the fact that I don't think we explain anywhere how the various types should be handled

I think a short section in the wrapper-specification.md should suffice.

Are you ok with the string selector type name?

[WasabiFan]

Yes, that type name sounds good.

Thanks for helping with the maintenance of the core infrastructure as well as the individual bindings -- I wouldn't be able to keep evolving and fixing the spec by myself!

[ddemidov]

Resolved by #99

[rhempel]

NEVER MIND - I see it's already been done

How about this: Right now the spec only allows one type for a property and for trigger it's string. We could add a readType and writeType instead of type. In most cases they would be the same. For properties like trigger, the writeType would still be string, the read type would be something like bracketed string array or something that means the currently selected value is surrounded by brackets. The autogen system would then be able to know that the corresponding sys file returns a list with the current value bracketed and act accordingly.