Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestHandler per type or per type&name #72

Closed
danielpeintner opened this issue Oct 25, 2017 · 9 comments
Closed

RequestHandler per type or per type&name #72

danielpeintner opened this issue Oct 25, 2017 · 9 comments

Comments

@danielpeintner
Copy link
Contributor

In the FPWD there is the possibility to define a RequestHandler for each type (namely onRetrieveProperty, onUpdateProperty, onInvokeAction, and onObserve) of an ExposedThing.

Each of those 4 handlers needs to deal with all possible properties/actions/events meaning that THE onRetrieveProperty handler for example needs to be able to handle all available properties such as the property toggle, brightness, RGB, ... in the case of a light example.

Having just one handler might reduce the code and simplify the spec but it also limits what we can do. For example we cannot

  • easily update a single handler for updating the property brightness
  • easily add handlers in different portions of the code

Moreover, I have the feeling it is also more intuitive to be able to specify a handler for each combination of each request (type&name).

Are there any opinions on whether the current FPWD approach is the right way to go or whether we want to change the WebIDL (e.g. for onRetrieveProperty)
ExposedThing onRetrieveProperty(RequestHandler handler);
to
ExposedThing onRetrieveProperty(DOMString popertyName, RequestHandler handler);
as we used to have.

@zolkis
Copy link
Contributor

zolkis commented Oct 25, 2017

we cannot easily update a single handler for updating the property brightness

In a running app we don't update a single handler during operation. We deploy the code that implements ExposedThing and then call start() on it.

we cannot easily add handlers in different portions of the code

That is true, all properties are handled in one function. The question is how different is handling per property in order to justify the overhead of using one handler per property? In my understanding, for the given requests (retrieve, update, [create, delete]) the handling of properties is very much the same, as it deals with object and property metadata, which is independent of the content of the property.

Could you provide examples that justify the one handler / property design for the WoT use cases?

@mkovatsc
Copy link
Contributor

I would classify all current definitions of ExposedThing.on* as bugs. They need to be set per individual Interaction. Standardizing some internal dispatcher API and let the user check and multiplex all incoming requests is just wrong when trying to define a simple, user-friendly API.

Again, we do not want to standardize how a complete software stack should look like, but a very simple API at the very top that makes the most out of the Thing abstraction. Everyone who wants more needs to dig into implementation details and adapt the code there.

I want to fix this by defining a mandatory delta for the Scripting API in the Burlingame Current Practices.

@mkovatsc
Copy link
Contributor

register() / unregister()

At the moment, it sounds like the script has to call register() before start(). Yet the whole part about "Generates the Thing Description" sounds out of place. From the Scripting API, we do not care if the WoT Runtime implementation writes out actual files or is able to generate TDs on the fly from the ExposedThing object instance.

register() should be a simple convenience function (if standardized at all) to register at a Thing Directory. In principle, the Directory could be a ConsumedThing on which the script call dir.invokeAction("register" ,thing.description).

start() / stop()

Yes, these make sense to avoid a WoT Client accessing an unfinished ExposedThing.

@zolkis
Copy link
Contributor

zolkis commented Oct 25, 2017

I would classify all current definitions of ExposedThing.on* as bugs.

Well, this only proves you have misunderstood them. Probably because the algorithms are sadly missing from the spec at this moment. Also, we could add the Request suffix to all onXxx() method, e.g. rename the onObserve() to onObserveRequest() to make it more clear it's a handler (only one per request type is allowed, as opposed to events that can have as many handlers as subscriptions).

register() should be a simple convenience function (if standardized at all) to register at a Thing Directory.

Yes, that is right, except that the design intent is to express developer intent to publish the Thing to a URL. Now it is a completely different question what service is behind that URL, only that is must be a TD publishing service. Besides, the whole scripting API is just a collection of convenience functions :), provided by a runtime that does some isolation job. However, there is nothing one can do with scripting and cannot do with bare WoT interactions.

@mkovatsc
Copy link
Contributor

mkovatsc commented Oct 25, 2017

If your intent is that onUpdateProperty() is one of those low-level function like onObserve() that enable features like logging or analytics within the script (which I want to put out of scope for now), then I indeed misunderstood it. Sorry!

Yet then, the Scripting API also forgot the actual handler to act upon a Property change, e.g., to adjust the brightness of the local LED hardware to the desired level. I guess ThingPropertyInit is missing a PropertyHandler handler; with PropertyHandler being a function with oldValue and newValue arguments. This let me to believe you want to have it in the on*() callbacks as before ;)

@zolkis
Copy link
Contributor

zolkis commented Oct 25, 2017

No, all these handlers define how to handle e.g. CRUDN requests, that's it. They also exist in other IoT stacks cover the functionality pretty well (we have a subset of CRUDN here). The onPropertyUpdateRequest() function provided by the app defines how to handle update requests of a given property, and so forth.

In the use case you mentioned it receives the new value requested by a client, and makes all local steps needed in order to set that value. If there are observers to that change (as defined by the onObserveRequest() handler), those clients will be notified as well.

@mkovatsc
Copy link
Contributor

We have a scoping issue if you want to standardize a CRUDN IoT framework, We need a simple scripting API on top of the Thing abstraction. A generic CRUDN Request API where the API users have to do all the multiplexing by themselves is not helping this key goal.

ThingActionInit has a Function action entry to define the functionality of the specific Action. Good.

ThingPropertyInit is missing a handler that can apply changes related to the state of the specific Property. Bug: users need to revert to another API, the low-level CRUDN level.

@zolkis
Copy link
Contributor

zolkis commented Oct 26, 2017

It's not any lower level than a property-specific handler would be. It's just multiplexed, that's it.
Could you provide example code what such a handler would do for a few kinds of properties, and see how much overlap there is.
Then it would become more understandable why there is a common property change request handler.
However, the feedback is noted and we'll make a group decision about it.

@zolkis
Copy link
Contributor

zolkis commented Feb 14, 2018

Obsoleted by #86.

@zolkis zolkis closed this as completed Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants