-
Notifications
You must be signed in to change notification settings - Fork 448
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
gatt - enable service changed characteristic #128
base: master
Are you sure you want to change the base?
Conversation
b1817d4
to
ea3975b
Compare
@sandeepmistry Ready for review! |
ea3975b
to
537a472
Compare
537a472
to
13ce04e
Compare
@@ -32,8 +32,7 @@ | |||
"devDependencies": { | |||
"jshint": "~2.3.0", | |||
"should": "~2.0.2", | |||
"mocha": "~1.14.0", | |||
"node-blink1": "~0.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing node-blink1
will break one of the examples ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it out because it wasn't building on Beaglebone, I'll add it back in.
@popasquat89 overall changes look good, mostly concerned about handle and attribute id management. Could you please clarify the use case for adding/removing services? Just like to better understand what you are using this feature for. |
I have a set of points that change periodically. I need the ability to solicit the same UUID for many different points, so when one is removed / added I'll have a way of telling. I'll think about a strategy for managing the handles. I think a simple method with some checks in place would work fine. |
@popasquat89 an alternative is to leave the API as is, just Thoughts? |
9aadb01
to
54323cc
Compare
I'm not thrilled with the method, but checkout my latest PR for mac. This will work, just have to replicate this on the hci side. As far as what you're talking about for the central, the product I'm building needs to be totally compliant with Bluetooth protocols, as well as have the capability of caching the handles. Unfortunately I can't go into too much detail, but having a good cache on our centrals is going to be crucial. Let me know what you think about the PR I just pushed, I'm working HCI now. |
a840872
to
be34355
Compare
@sandeepmistry Ready for you to take a look once again, if you give it the go ahead I can squash down for merge. |
3583a7b
to
2785721
Compare
Hey @sandeepmistry , anything you want me to change for this pr? |
@popasquat89 nothing yet. I need to find an hour or 2 to try out your changes. I'd like to understand the behaviour on OS X with handles and compare it to the Linux implementation. Ideally both would behave the same. If you have any notes on this that you could share, that would speed up the process. |
@sandeepmistry I think I understand your question, so I'll answer to the best of my knowledge. On the OS X side, we don't have much control over the micro operations that are occurring with the handles. We are still storing them in the same fashion as we do on the linux side, just leaving out descriptors on the characteristics since we are including them as a object to be passed into core bluetooth.(descriptors don't receive an attribute id) This is all stuff that I'm sure you know, the only difference of the way we handle the add / remove operations on the platforms is with the service changed characteristic. On linux, we need to change this characteristic value and notify whenever we add or remove the service. If we remove all of the services, in Linux we change service changed characteristic value to 0 - 0xFFFF in order to tell the central that it should rescan all services. If we just remove / modify a service, just pass in the handles for that one service into the service changed characteristic. This will notify the central to invalidate those handles and rescan if needed. On OS X, the service changed characteristic is handled by the xpc API. The most important thing on the bleno side is to keep track of our records, and make sure that we keep a clean record of which services are still valid. In both cases we are seeing the same API for bleno, so the user can remain ignorant to how the API is implemented. Some important sections in the specification that may be of interest: Bryce |
So I briefly had a chance to start looking at OS X CoreBluetooth peripheral behaviour when removing and adding a service. See the following branch: https://github.com/sandeepmistry/osx-ble-peripheral/tree/add-remove-services When I connect using gatttool from Linux, and list the primary services before and after:
You can see the 0x20 -> 0x22 handles are re-used. I think we should take look at a few more scenarios, to better understand how OS X manages handles. Then apply the knowledge to the Linux side. We also need to see what happens when the same service is added, removed and added, to see if the same @popasquat89 hopefully this clarifies my question from before. |
@sandeepmistry This is actually intended functionality. Let me explain my procedure of adding a service. If there is to be another service added, I check to see if the attribute / handle impact will put the total count over 0xFFFF and do some special functionality if it is. My guess, is that it's firing when it's not supposed to. This will be an easy fix. Ref: What I'm doing is checking if the handle / attribute count is over 0xFFFF, and if it is I'm grouping all the openings that exist in our attribute / handle array to see the best place to stick our new service. If we find an exact match for the amount of handles our new service needs, we place the service in that hole. If we don't find an exact match, let us just go with the next largest that can host. Example: Say our handles looks like the following. [ (0x000) When the new service is to be added, we will go through and scan for openings of where old services have been removed. It will look like the following. { In this case, if our service has an attribute / handle impact of 4, it will be placed in the first available hole in our attribute / handle array with 4 openings which happens to be index 5-8. I believe this method will be more than sufficient for handling rollover and new services. The statement just needs to be fixed to not execute whenever we aren't rolling over at 0xFFFF. |
Hm, upon further inspection I see what you're referring to. I'm wondering if blued doesn't allow gaps in the handles array that they manage. I'll look into this and try to provide a more consistent API across both Mac and Linux. |
@popasquat89 further investigation would be great! I think we just need to learn about how a few more scenarios are handled, for example:
Also, it would be good to see what the indications look like. Especially is many services are added or removed in a row, does it wait a bit before sending an indication. |
@sandeepmistry Check this out, I put together as many edge cases as I thought were necessary. Scenario 1: Remove first service, check that all handles don't get shifted forward.
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
Conclusion: Handles don't changeScenario 2: Remove middle service
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215 Conclusion: Service 2 is successfully removed and other handles stay the same as they were previously to removing service 2.Scenario 3: See what happens when previously removed service is re-added.
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
attr handle: 0x002d, end grp handle: 0x002f uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214 Conclusion: The service is added to the end of the handles array, all other handles stay the same.Scenario 4: Try to add a service that is already on being hosted by the handles array.
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
attr handle: 0x002d, end grp handle: 0x002f uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214 Conclusion: If you try to add a service with the same attribute id that is currently on the handles array, an xpc error will be thrown.Scenario 5: Try removing the last service in the handles array, to see if the next service is shifted to the first available handle (service that was just removed) or the next available handle (handle that would be after the service that was just removed)
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0024, end grp handle: 0x0026 uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
attr handle: 0x002a, end grp handle: 0x002c uuid: 27e6df9e-57ec-4f98-95fb-59b15c3d0215
[20:C9:D0:49:83:30][LE]> primary
attr handle: 0x0001, end grp handle: 0x0005 uuid: 00001800-0000-1000-8000-00805f9b34fb
attr handle: 0x0006, end grp handle: 0x0009 uuid: 00001801-0000-1000-8000-00805f9b34fb
attr handle: 0x0010, end grp handle: 0x001f uuid: 0000180a-0000-1000-8000-00805f9b34fb
attr handle: 0x0020, end grp handle: 0x0023 uuid: d0611e78-bbb4-4591-a5f8-487910ae4366
attr handle: 0x0027, end grp handle: 0x0029 uuid: 27e6df9f-57ec-4f98-95fb-59b15c3d0214
attr handle: 0x002a, end grp handle: 0x002c uuid: bd0f6577-4a38-4d71-af1b-4e8f57708080 Conclusion: The only time that the handles array will "shift" to a previous services' spot is when it's the last service on the handles array. In this case, since Service 3 was removed, and there were no more services on the array, the service 1 that was added back is added to where service 3 was previously.We can fix this, by simply scanning the handles array to see where the last service is and add to our handles array from that point. I want to say that they're probably using a linked list, that has an handle id associated with each node. Whenever a link in the chain is taken out, the last element has a handle id that would be reminiscent of before the last service was added. Imo, this isn't so good when we're talking about attribute caching, I wonder if this is intended functionality on their part. Edge case for sure, but does go against caching when they are adding a service in the place of where our old service was. Let's say I have some devices in the wild that are connecting every x cycles and are looking to query a service that was there before, this will hurt that functionality. I'll put together a patch to fix the functionality and have linux do the same. Let's land this puppy! Bryce |
bd0cf98
to
61dd8e7
Compare
gatt addService: added handles 24-35 +3ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt removeService - removed handles: 24-35 +5ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt addService: added handles 24-35 +23ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt removeService - removed handles: 24-35 +3ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt addService: added handles 24-35 +3ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt removeService - removed handles: 24-35 +4ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt addService: added handles 24-35 +3ms
gatt sendATTMessage: indicate-29090024003500 +2s
gatt removeService - removed handles: 24-35 +4ms Ok, results from making linux addition of services resemble os x behavior. Indicate message that's being sent is also in log, let me know what you think. |
@popasquat89 sweet! Thanks for researching this! Great summary!
I've never been too keen about caching, most likely the handles will never change in the real world, maybe only with a firmware update. On the Linux side, I think there's one last change that is needed, it's currently always sending an indication now, even if the central hasn't enabled them for the 0x2a05 characteristic. I believe it should only be sent if the central enabled indications right? On the OS X side, I think it needs to be updated to re-use the existing attribute ID for a previous service/characteristic/descriptor instead of creating a new one like CoreBluetooth does. Thoughts? |
@sandeepmistry In the case of what we're doing, we have services solicited with the same UUID that have handles being changed consistently. The caching comes into play for services that aren't changed and important to other devices in the wild. I'll be sure to look into that fix for linux, should be straightforward. For OS X, I think we should have it resemble the Linux side as closely as possible. Right now on Linux when a service is added, it's added to the end unless it's handle impact will cause the handles array to roll over 0xFFFF. If we add it to the end on Linux, we should on OS X as well. If we want to have it re-use a set of attribute Id's that were previously commissioned for a prior service that has been removed, then we should imitate that on the Linux side with our handles. If I look at the problem from the perspective of caching, it makes more sense to me to add onto the array until we run out of space and only then running special operations to find open spaces in our array to fit a new service. If you're talking about re-using an attributeId for if the same instanced service is re-added (I don't think you are), I would say that's going to be tough unless you save the same instanced service to pass into the addService method. The reason being, if I have the same service being used multiple times, the way that it's looked up is by the attribute ID set as a field inside that specific instance so we can tell services with the same UUID apart. This would also require a person to hang on to the same instanced service while it's not in use. I guess this is theoretically feasible, just not sure what one would gain since the we have already told central devices to invalidate their cache with the service changed characteristic. Bryce |
Fixed indicate: gatt addService: added handles 21-31 +1s
gatt removeService - removed handles: 21-31 +2s
gatt addService: added handles 21-31 +2s
gatt removeService - removed handles: 21-31 +2s
gatt addService: added handles 21-31 +2s
gatt removeService - removed handles: 21-31 +2s
gatt addService: added handles 21-31 +2s
gatt removeService - removed handles: 21-31 +2s
gatt addService: added handles 21-31 +2s
gatt removeService - removed handles: 21-31 +2s
gatt addService: added handles 21-31 +2s |
@sandeepmistry Anything you would like me to do to land this? |
@popasquat89 sorry for the delay, I was travelling for a bit and still catching up on things. This is still on my todo list, I'm hoping to try this out either tomorrow or next weekend ... |
@popasquat89 I've started merging this into the following branch: https://github.com/sandeepmistry/bleno/tree/add-remove-services So far only tested OS X, but it looks good. I'm wondering if we should add optional callback args to the I'm probably have to look at the Linux side later this week though. |
I think that would be a good idea, I'm using a modified version of your node-bluetooth-hci that has a pretty clean abstraction for async functionality. I'll take a look into implementing this today. |
I take that back, I'm forgetting that the GATT functionality we tampered with doesn't send socket commands...it's early here. I just went with the prescribed way that is seen in setServices. |
@sandeepmistry Is there anything else you needed me to do in order to get this merged? |
Hey @sandeepmistry I'm doing some library work this week, as we're prototyping a new system with Bleno. What will it take for me to punch through this update? Bryce |
@sandeepmistry Is there a chance that this can be merged in the near future? Would be very nice to see this supported. |
I'd really appreciate this getting merged and available in the NPM if possible. It seems the only time android flushes the cache is when it sees a service change notification Anything I can do to help ? |
@sandeepmistry Long time no speak! Any chance I can do something to get this merged in? I'm prototyping another board for work and could really use the features that have been merged in since this PR! |
We are approaching the 2 year mark on this one, @sandeepmistry can we have an ETA on when will this get merged? |
May I suggest to forward your changes to: https://github.com/abandonware/bleno/ Meanwhile, feedback welcome on latest release: |
This commit adds the ability for the Changed Service Characteristic to be used. Ref Bluetooth Spec 3.G.7.1
This service is already on the base services, I'm just giving it functionality. This will also enable attribute caching on any central device that supports it.
Please let me know what you think.
Thanks,
Bryce Jacobs