You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/11/15 16:49:54 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #2302: bluetooth: fix outgoing buffer freeing
v01d opened a new pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302
## Summary
This fixes a regression introduced in #2070 where the outgoing HCI message buffer was not freed anymore. At the same time, that PR introduced this buffer freeing step for the simulated HCI socket, right after it was sent to the real BT stack, since when NuttX's own bluetooth stack is disabled, at this point the buffer needs to be freed. These two changes compensated each other when NuttX's stack was disabled, but when enabled it resulted in the buffer being released at the wrong time.
This reinstates the buffer freeing at the correct place needed for NuttX BLE stack and conditions the freeing of the buffer in the simulated HCI socket to only be done when NuttX stack is disabled.
## Impact
Fixes regression, observed when NuttX's BLE stack is used with the sim HCI socket.
## Testing
sim:bthcisock
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727607446
>
>
> We cannot be making changes in the bottom half HCI interface to support nimBLE. Won't this impact the non sim drivers as well?
If nimBLE requires changes to HCI drivers, then we will have to create a different driver interface spec and implement a different set of drivers (or add conditional logic in existing drivers).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748090104
I think the agreement is to support stack like bluez like which use L2CAP and HCI in kernel side, If we want to support nimBLE, what we agree it that nimBLE must remove L2CAP/HCI and call the full feature bluetooth socket instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748122021
> > HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
>
> That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?).
> In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Yes, your concern is right, but from API compability perspective, it's better to decouple RAW socket from disabling NuttX stack:
1. User can use RAW socket regardless whether NuttX bluetooth stack enable or disable
2. Support WIRELESS_BLUETOOTH_HOST option too
>
> Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
>
> * fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
Agree, we need make NuttX BLE stack work again. With or without WIRELESS_BLUETOOTH_HOST.
> * have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
>
> * continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
>
> Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
I wouldn't suggest to modify all monolithic bluetooth stack too much(remove L2CAP and HCI layer). The possible solution is that we select one profile only bluetooth stack(bluez?) directly or modify one monolithic bluetooth stack(nimBLE or zerphyr?) heavily to utilize the full feature bluetooth socket. And then the selected stack become NuttX userspce native bluetooth stack.
All rest monolithic bluetooth stack can use either TPROTO_* raw socket or /dev/tty* interface to access the BT/BLE controller. It doesn't have too much difference from the archecture perspective for these two approach since NuttX native kernel bluetooth stack(L2CAP and HCI) get bypass always, the only difference is how to bypass NuttX bluetooth stack:).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729066228
Well, the allocated buffer needs to be freed eventually, don't see why that it is odd. The only reason why this isn't done there for both cases is NuttX's stack need to keep the buffer around for some more time. It is a consequence of having the host stack deal with BT buffers to implement the protocol in contrast to an implementation via sockets where data is handled externally (ie, the buffer only lives to be transfered between network to driver layer and viceversa).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729078791
I don't understand what you mean as nimBLE runs on top of the socket.
I think the actual issue is that NuttX's own stack is not very well integrated to NuttX: there's actual support for L2CAP sockets but NuttX host layer bypasses this and send commands directly using bt_* function calls. That is the actual problem. If NuttX's layer used the socket interface this distinction wouldn't be needed. This is the difficulty I have while trying to maintain compatibility with NuttX stack.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727623190
I guess the nimBLE porting layer could be modified so that it frees the buffer itself. It is kind of annoying to do it this way but it is possible. It may take a few weeks before I go back to nimBLE though (I will also ping the nimBLE devs since the PR there has been really quiet). I can revisit this when I get back to that. In the meantime you can fix NuttX stack by just taking the change in hcicore and leave the other one out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748190718
Yeah sorry I have been really busy during the week with work closing out the year. I'll look this weekend at that reference issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748122021
> > HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
>
> That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?).
> In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Yes, your concern is right, but from API compability perspective, it's better to decouple RAW socket from disabling NuttX stack:
1. User can use RAW socket regardless whether NuttX bluetooth stack enable or disable
2. Support WIRELESS_BLUETOOTH_HOST option too
>
> Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
>
> * fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
Agree, we need make NuttX BLE stack work again. With or without WIRELESS_BLUETOOTH_HOST.
> * have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
>
> * continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
>
> Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
I wouldn't suggest to modify all monolithic bluetooth stack too much(remove L2CAP and HCI layer). The possible solution is that we select one profile only bluetooth stack(bluez?) directly or modify one monolithic bluetooth stack(nimBLE or zerphyr?) heavily to utilize the full feature bluetooth socket.
All rest monolithic bluetooth stack can use either TPROTO_* raw socket or /dev/tty* interface to access the BT/BLE controller. It doesn't have too much difference from the archecture perspective for these two approach since NuttX native kernel bluetooth stack(L2CAP and HCI) get bypass always.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748087792
> BTW, to support the monolithic(profile plus l2cap/hci) bluetooth stack like zerphyr and nimBLE, the serial driver(/dev/tty*) is better interface. Here is how bluez and bluedroid work on Linux:
>
> 1. bluez contain profile only, and talk with kernel l2cap/hci through socket
>
> 2. bluedroid contain profile+l2cap+hci talk with kernel through /dev/tty*
>
>
> It's strange and complex to support bluedroid/nimBLE through the raw/managed bluetooth socket but bypass L2CAP/HCI layer.
There was already a discussion on this before and there was agreement that the way to support a BLE stack and expose a BLE controller was via networking interfaces, not custom character drivers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729047833
I still think it's a little odd that we are having to add this special case here. Seems like this is a problem for anything sending via the raw channel?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729074040
But I should be able to run nimBLE on top of the socket regardless just as we do with Linux. There is something ugly happening here if we need to change the free logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748122021
> > HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
>
> That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?).
> In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Yes, your concern is right, but from API compability perspective, it's better to decouple RAW socket from disabling NuttX stack:
1. User can use RAW socket regardless whether NuttX bluetooth stack enable or disable
2. Support WIRELESS_BLUETOOTH_HOST option too
>
> Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
>
> * fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
Agree, we need make NuttX BLE stack work again. With or without WIRELESS_BLUETOOTH_HOST.
> * have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
>
> * continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
>
> Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
I wouldn't suggest to modify all monolithic bluetooth stack too much(remove L2CAP and HCI layer). The possible solution is that we select one profile only bluetooth stack(bluez?) directly or modify one monolithic bluetooth stack(nimBLE or zerphyr?) heavily to utilize the full feature bluetooth socket.
All rest monolithic bluetooth stack can use either TPROTO_* raw socket or /dev/tty* interface to access the BT/BLE controller. It doesn't have too much difference from the archecture perspective for these two approach since NuttX native kernel bluetooth stack(L2CAP and HCI) get bypass always, the only difference is how to bypass NuttX bluetooth stack:).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748090104
> There was already a discussion on this before and there was agreement that the way to support a BLE stack and expose a
I think the agreement is to support stack like bluez like which use L2CAP and HCI in kernel side, If we want to support nimBLE, what we agree it that nimBLE must remove L2CAP/HCI and call the full feature bluetooth socket instead.
> BLE controller was via networking interfaces, not custom character drivers.
it isn't a custom character drivers, it's the standard tty driver.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748067949
BTW, to support the monolithic(profile plus l2cap/hci) bluetooth stack like zerphyr and nimBLE, the serial driver(/dev/tty*) is better interface. Here is how bluez and bluedroid work on Linux:
1. bluez contain profile only, and talk with kernel l2cap/hci through socket
2. bluedroid contain profile+l2cap+hci talk with kernel through /dev/tty*
It's strange and complex to support bluedroid/nimBLE through the raw/managed bluetooth socket but bypass L2CAP/HCI layer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748105378
> > > @v01d and @btashton NuttX native bluetooth stack is broken now. I think we should restore the functionality before we find a better solution. It's wrong to support nimBLE but break the native stack for the long time.
> >
> >
> > The fact that the NuttX stack broke was not due to a mistake during previous PR, which this PR attempted to fix. As @btashton considered the change to not be fully correct this PR was not merged, until he could look into the problem. It is not a matter of prioritizing nimBLE over NuttX stack.
>
> I think current NuttX stack is broken by this patch:
>
> ```
> commit 5386f972fa8020b69798ab70325b7f4df0f20cf3
> Author: Matias N <ma...@protobits.dev>
> Date: Sun Sep 20 16:45:09 2020 -0300
>
> bluetooth: Add support for HCI RAW channel; make host layer optional
> ```
>
> HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?). In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
- fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
- have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
- continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729127051
I agree that the existing stack needs to be adjusted a bit but you will still have packets that are not at L2CAP level.
I suspect we just need to be increasing the ref count on the packets so they can both be released at the same point.
I can look at this, but it will be a little bit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748937378
let's call this PR and focus on the new one: https://github.com/apache/incubator-nuttx/pull/2571
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748122021
> > HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
>
> That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?).
> In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Yes, your concern is right, but from API compability perspective, it's better to decouple RAW socket from disabling NuttX stack:
1. User can use RAW socket regardless whether NuttX bluetooth stack enable or disable
2. Support WIRELESS_BLUETOOTH_HOST option too
>
> Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
>
> * fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
Agree, we need make NuttX BLE stack work again.
> * have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
>
> * continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
>
> Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
I wouldn't suggest to modify all monolithic bluetooth stack too much(remove L2CAP and HCI layer). The possible solution is that we select one profile only bluetooth stack(bluez?) directly or modify one monolithic bluetooth stack(nimBLE or zerphyr?) heavily to utilize the full feature bluetooth socket.
All rest monolithic bluetooth stack can use either TPROTO_* raw socket or /dev/tty* interface to access the BT/BLE controller. It doesn't have too much difference from the archecture perspective for these two approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727605105
We cannot be making changes in the bottom half HCI interface to support nimBLE. Won't this impact the non sim drivers as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748063532
@v01d and @btashton NuttX native bluetooth stack is broken now. I think we should restore the functionality before we find a better solution. It's wrong to support nimBLE but break the native stack for the long time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748094267
> I think the agreement is to support stack like bluez like which use L2CAP and HCI in kernel side, If we want to support nimBLE, what we agree it that nimBLE must remove L2CAP/HCI and call the full feature bluetooth socket instead.
There was no discussion on conditioning nimBLE to change how it works for it to be supported in NuttX (this was mentioned as a future possibility only). I've been working for various months on supporting nimBLE now and this was not pointed out before as a required condition to be included. Moreover, changes to NuttX were to support RAW HCI sockets, not condition it to work for nimBLE specifically.
What we need to actually address is the fact that NuttX stack does not even use sockets properly, so any discussion regarding a proper interface should start there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727605105
We cannot be making changes in the bottom half HCI interface to support Zephyr. Won't this impact the non sim drivers as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748067949
BTW, to support the monolithic(profile plus l2cap/hci) bluetooth stack like zerphyr and nimBLE, the serial driver(/dev/tty*) is better interface. Here is how bluez and bluedroid work on Linux:
1. bluez contain profile only, and talk with kernel l2cap/hci through socket
2. bluedroid contain profile+l2cap+hci talk with kernel through /dev/tty*
It's strange and complex to support bluedroid through the raw/managed bluetooth socket but bypass L2CAP/HCI layer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748200857
I was just finishing up a proper nimBLE config for sim in #2129 so that nimBLE can be easily tested. There was a needed fix on apps side which should now make #2129 work (I just restarted CI on that PR). Once that is done, you can use sim:nimble config (see the README on that PR on how to test).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748665557
you need to raise the interface first with `ifup bnep0`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748669420
Thanks I need to do a little more testing this evening, but I have a change to this patch that I think cleanly resolves the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748199058
@v01d is there place I can look for what I need to set up your nimBLE configuration via the sim? That way I can make sure things work correctly in both modes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748665135
@v01d I just tried to do some testing and I could not get the nimble configuration to actually do anything.
I see the HCI device attach but when I start the nimble app it starts the tasks but nothing actually is send over the HCI interface. I added some scanning into the app and that did not seem to change anything.
```
NuttShell (NSH) NuttX-10.0.1
nsh> ?
help usage: help [-v] [<cmd>]
. cp exit ifup mkrd ps test xd
[ cmp false kill mh pwd time
? dirname free losetup mount rm true
basename dd help ls mv rmdir uname
break df hexdump mb mw set umount
cat echo ifconfig mkdir nslookup sleep unset
cd exec ifdown mkfatfs poweroff source usleep
Builtin Apps:
hello nimble nsh sh
nsh> nimble &
nimble [5:255]
hci init
port init
gap init
gatt init
ans init
ias init
lls init
tps init
hci_sock task init
ble_host task init
nsh> hci sock task
host task
nsh> ps
PID PPID PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK COMMAND
0 0 0 FIFO Kthread N-- Ready 00000000 004096 Idle Task
1 0 224 FIFO Kthread --- Waiting Signal 00000000 065552 hpwork
2 0 100 FIFO Kthread --- Waiting Signal 00000000 065552 lpwork
3 0 100 FIFO Task --- Running 00000000 004112 init
4 3 100 FIFO Kthread --- Waiting MQ empty 00000000 065552 BT HCI Tx
5 3 255 RR Task --- Waiting Signal 00000000 016400 nimble
6 3 1 RR pthread --- Waiting MQ empty 00000000 065584 <pthread> 0x0
7 3 1 RR pthread --- Waiting Semaphore 00000000 065584 <pthread> 0x0
```
btmon
```
= Close Index: 04:D3:B0:FC:BE:16 [hci0] 420.521753
@ MGMT Event: Index Removed (0x0005) plen 0 {0x0001} [hci0] 420.522192
= Open Index: 04:D3:B0:FC:BE:16 [hci0] 420.522319
= Index Info: 04:D3:B0:FC:BE:16 (Intel Corp.) [hci0] 420.522326
@ RAW Close: nuttx {0x0002} 420.522333
@ USER Open: nuttx (privileged) version 2.22 {0x0002} [hci0] 420.522338
= bluetoothd: Endpoint unregistered: sender=:1.107 path=/MediaEn.. 420.524184
= bluetoothd: Endpoint unregistered: sender=:1.107 path=/MediaEn.. 420.524293
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748090370
> @v01d and @btashton NuttX native bluetooth stack is broken now. I think we should restore the functionality before we find a better solution. It's wrong to support nimBLE but break the native stack for the long time.
The fact that the NuttX stack broke was not due to a mistake during previous PR, which this PR attempted to fix. As @btashton considered the change to not be fully correct this PR was not merged, until he could look into the problem. It is not a matter of prioritizing nimBLE over NuttX stack.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727604759
Doesn't this suggest that all the other Bluetooth drivers are also now broken?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727604885
What do you mean by bluetooth drivers?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748100053
> > @v01d and @btashton NuttX native bluetooth stack is broken now. I think we should restore the functionality before we find a better solution. It's wrong to support nimBLE but break the native stack for the long time.
>
> The fact that the NuttX stack broke was not due to a mistake during previous PR, which this PR attempted to fix. As @btashton considered the change to not be fully correct this PR was not merged, until he could look into the problem. It is not a matter of prioritizing nimBLE over NuttX stack.
I think current NuttX stack is broken by this patch:
```
commit 5386f972fa8020b69798ab70325b7f4df0f20cf3
Author: Matias N <ma...@protobits.dev>
Date: Sun Sep 20 16:45:09 2020 -0300
bluetooth: Add support for HCI RAW channel; make host layer optional
```
HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-748122021
> > HCI RAW socket is selected by WIRELESS_BLUETOOTH_HOST, but the right implementation should enable RAW mode per socket instance. If we look at how Linux bluetooth stack work, the RAW bluesocket don't require us to remove bluetooth stack and recompile kernel, so the RAW bluetooth socket and NuttX native bluetooth stack isn't mutually exclusive.
>
> That is indeed another option, to support this per-socket. I personally don't think that makes much sense in an embedded system were you will have one BLE stack (why would you want to support RAW sockets to be used by an external stack and still have NuttX stack enabled?).
> In other words, I don't oppose to doing that but I would maintain the option to disable NuttX stack as it would otherwise be wasteful when an external stack is used.
Yes, your concern is right, but from API compability perspective, it's better to decouple RAW socket from disabling NuttX stack:
1. User can use RAW socket regardless whether NuttX bluetooth stack enable or disable
2. Support WIRELESS_BLUETOOTH_HOST option too
>
> Anyway, this goes beyond the fix that is needed for the problem mentioned in this PR, which is relatively simpler compared to that. I think we need to break down this as follows:
>
> * fix the buffer freeing and restore NuttX current BLE stack functionality (@btashton are you able to look into this as you mentioned?)
Agree, we need make NuttX BLE stack work again. With or without WIRELESS_BLUETOOTH_HOST.
> * have a discussion on what do we want to do with NuttX BLE stack to properly support it using sockets (modify it? replace it?)
>
> * continue improving BTPROTO_* socket handling in NuttX (Brennan had further plans with this)
>
> Meanwhile, having nimBLE supported at least gives us a mature stack exposed via sockets in NuttX.
I wouldn't suggest to modify all monolithic bluetooth stack too much(remove L2CAP and HCI layer). The possible solution is that we select one profile only bluetooth stack(bluez?) directly or modify one monolithic bluetooth stack(nimBLE or zerphyr?) heavily to utilize the full feature bluetooth socket.
All rest monolithic bluetooth stack can use either TPROTO_* raw socket or /dev/tty* interface to access the BT/BLE controller. It doesn't have too much difference from the archecture perspective for these two approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729016848
I went over this again. I'm now (hopefully) addressing both issues correctly. These were actually unrelated: the missing release on receive queue was simply a mistake. As for the release on transmit (which I added to the sim BT device) is now placed inside bt_send(), which I think is the correct place since this is the last point of the upper-half of BT drivers where the buffer will be seen. This is only done when NuttX's stack is disabled, since as I mentioned NuttX's stack needs to keep the buffer around, whereas for an external BT stack, this buffer is hidden inside networking layer so it needs to be released once sent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-727607446
>
>
> We cannot be making changes in the bottom half HCI interface to support nimBLE. Won't this impact the non sim drivers as well?
If nimBLE requires changes to HCI drivers, then we will have to create a different driver interface spec and implement a different set of drivers (or add conditional logic in existing drivers).
... assuming that the different drivers requirements cannot be made compatible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2302: bluetooth: fix outgoing buffer freeing
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2302:
URL: https://github.com/apache/incubator-nuttx/pull/2302#issuecomment-729131978
Ok, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org