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

Throw an InvalidStateError when an invalid attribute is used. #272

Closed

Conversation

jyasskin
Copy link
Member

@jyasskin jyasskin commented Aug 10, 2016

@jyasskin jyasskin force-pushed the invalidated-attributes branch 4 times, most recently from c87a0dc to f6106e9 Compare August 12, 2016 00:33
</li>
</ol>
</li>
<li>
Let <var>changedServices</var> be a set of <a>Service</a>s, initially empty.
</li>
<li>
Copy link
Contributor

@g-ortuno g-ortuno Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In step 4 it says:

If attribute [...] appears in both, [...] remove it from both.

But in step 8 it says:

If service [...] appears in both remove it from both and add it to changedServices.

Maybe I'm missing something but wouldn't step 4 remove all services that step 8 would have added to changedServices?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 4 intends to remove attributes whose only changes are in their values. "with the same definition" may not be clear enough about this.

Copy link
Contributor

@g-ortuno g-ortuno Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. Though, Step 2 says to use

Primary Service Discovery, Relationship Discovery, Characteristic Discovery, and Characteristic Descriptor Discovery procedures

None of those procedures reads the values of the Characteristics or Descriptors. If I understand correctly you only get the Value Handles.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a service like:

Before:

  • Service A:
  • Characteristic B

After:

  • Service A:
    • Characteristic C

Then step 4 doesn't remove Service A from removedAttributes or addedAttributes since it has a different definition (the list of included services and characteristics is part of a service definition per 3.G.3.1), step 8 does remove it, and I believe no other step would remove it.

I think Included Services force step 8 to include the "add it to changedServices." bit. For Characteristics, I think step 9 would cover it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Then yeah I think a link to what it means to have the same definition would make things clearer. But also not related to this patch so OK to do in follow up

@g-ortuno
Copy link
Contributor

lgtm

@jyasskin jyasskin closed this Aug 15, 2016
@jyasskin jyasskin deleted the invalidated-attributes branch August 15, 2016 17:18
@g-ortuno
Copy link
Contributor

Did you mean to merge instead of closing?

@jyasskin
Copy link
Member Author

I did merge, as c8c9c52, 5fec7a4, and 001c3a8, but I confused github by pushing in the wrong order. I'll also refine the ServiceChanged steps in another patch.

@g-ortuno
Copy link
Contributor

SG. Could you rebase the other pull request?

@jyasskin
Copy link
Member Author

Done.

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

Successfully merging this pull request may close these issues.

Describe what to do if a method is called on a removed attribute
2 participants