[spec] led::trigger attribute about ev3dev-lang HOT 10 CLOSED

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?

from ev3dev-lang.

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.

from ev3dev-lang.

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;

from ev3dev-lang.

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?

from ev3dev-lang.

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.

from ev3dev-lang.

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.

from ev3dev-lang.

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?

from ev3dev-lang.

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!

from ev3dev-lang.

Resolved by #99

from ev3dev-lang.

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.

from ev3dev-lang.