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

Consider removing the value field from Characteristics and Descriptors #37

Closed
jyasskin opened this issue Sep 10, 2014 · 7 comments
Closed

Comments

@jyasskin
Copy link
Member

In discussion on dev-webapi@mozilla, @ehsan argued that

The apps can cache the value property themselves if they want to. These types of weird cache only properties don't have any prior art. In general, if something is as simple as this (by setting an expando property) to implement in the application code in JS, we should try to avoid adding it to the API.

We can also add the cache later if we find everyone using it.

Removing this field would mean that the notification/indication events need to include an ArrayBuffer in their fields, in addition to the Characteristic or Descriptor, but that's not terrible.

@armansito
Copy link

No, not terrible. Keep in mind though that not having a cache means that accessing a characteristic value might result in many read requests to the same characteristic if multiple web apps want to access it. For characteristics whose value is known not to change frequently, the cached value has the advantage of avoiding an actually over-the-air request (and battery drain).

So, this is something to consider. If a value has been read, I see no harm in caching it and most platform implementations already provide a cached value. So, why not just make it accessible through the API?

@jyasskin
Copy link
Member Author

Saving redundant reads across web apps sounds maybe-useful. (But how often will multiple apps be asking for the same value at similar times?) Do applications need to know how old the cached value is in order to use this? Would it make sense to have the application specify a maximum age for a cached value in the read() call, and then we could have a uniform interface for cached and uncached data? I think I'd still be inclined to wait for V2 for that.

@armansito
Copy link

Oh no, I wouldn't go that far at all (i.e. bother with a maximum age or anything). The way most APIs do this is to just provide the most recently read or notified value. Based on the profile, if an app knows that this value won't change (or will be notified when it does change), then they can just refer to the cached value.

For example, several apps might be interested in knowing the current battery level. The battery-level characteristic has both "read" and "notify" properties, so the app can just enable notifications from the characteristic and wait for the next update, but then also set up its initial state based on the cached value instead of sending out a read request.

And you have characteristics which may contain just long strings that won't really change frequently, so a cached value would be useful there as well.

So, there is definitely value in providing the cached value and most native APIs already support it. I don't have a strong opinion on this, so I'd let others weigh in as well.

@jracle
Copy link

jracle commented Sep 22, 2014

I'll just comment briefly as @armansito asks for others to reply on that issue, that I'm 100% aligned with his comments. We should keep cached value for the reasons he gives in his last reply.

@jyasskin
Copy link
Member Author

'k. @ehsan or @marcoscaceres, I'm leaning toward keeping value. Speak up if you still want to kill it.

@marcoscaceres
Copy link
Contributor

(will take a look in a day or two, currently triaging a week of unread email!)

@marcoscaceres
Copy link
Contributor

I think what @armansito is describing is essentially correct. Apart from first read, generally these values could be cached. Keep it for now and we can see how we go.

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

4 participants