You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "ldube (via GitHub)" <gi...@apache.org> on 2023/05/03 18:23:54 UTC
[GitHub] [nuttx] ldube opened a new pull request, #9166: wireless/bluetooth: Support removable bluetooth modules.
ldube opened a new pull request, #9166:
URL: https://github.com/apache/nuttx/pull/9166
## Summary
This bluetooth stack remains in an inconsistent state when the bluetooth HCI module is removed. This change adds a bt_netdev_unregister function that can be used to clean up after a module is removed. Some global variables are also set to their default values.
## Impact
## Testing
Some out-of-tree code was used for testing.
1) Confirmed that BT was still functional on the tm4c1294-launchpad with CC2564 module attached.
2) "Bluetooth Hotplugging" was tested on sama5d3-xplained using an out-of-tree USB BT HCI driver. The USB driver calls bt_netdev_unregister when the USB BT dongle is unplugged. Without rebooting, the linux gatttool is able to connect to NuttX BT when the USB BT dongle is plugged in again. By adding some debug printfs to bt_buf.c, I was able to confirm that all buffers, allocated during insertion, are released when the dongle is unplugged.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] ldube commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1534006811
>
Yes, only one HCI device is supported at the moment. I have thought about fixing this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #9166:
URL: https://github.com/apache/nuttx/pull/9166
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] btashton commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "btashton (via GitHub)" <gi...@apache.org>.
btashton commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1536374013
> @acassis No. I pushed several changes after creating the pull request. I am in sync with ldube:master and 839109f482ea84b9c5c79d7aa141822e11f05e9a builds without issues on my machine.
Can you build the sim/nimble config? I think some of these changes are not compatible for example txstatus is behind the host config flag
https://github.com/apache/nuttx/blob/839109f482ea84b9c5c79d7aa141822e11f05e9a/wireless/bluetooth/bt_hcicore.h#L72-L105
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] ldube commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1536416419
> > @acassis No. I pushed several changes after creating the pull request. I am in sync with ldube:master and [839109f](https://github.com/apache/nuttx/commit/839109f482ea84b9c5c79d7aa141822e11f05e9a) builds without issues on my machine.
>
> Can you build the sim/nimble config? I think some of these changes are not compatible for example txstatus is behind the host config flag
>
I think that is why. I don't understand why this CONFIG_WIRELESS_BLUETOOTH_HOST feature has to impose itself on such a simple little BLE 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1533991618
@ldube does bluetooth stack just support one HCI device? I saw the code destroy the core data structure in bt_netdev_unregister. Can we improve this situation?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on a diff in pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#discussion_r1184111610
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1444,6 +1453,7 @@ static void cmd_queue_init(void)
nxsem_init(&g_btdev.ncmd_sem, 0, 1);
g_btdev.ncmd = 1;
+ g_btdev.tx_status = 0;
Review Comment:
OK is 0, so it will be more meaningful.
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1575,6 +1593,53 @@ int bt_driver_register(FAR struct bt_driver_s *btdev)
void bt_driver_unregister(FAR struct bt_driver_s *btdev)
{
+ int ret;
+ FAR struct bt_buf_s *buf;
+
+ /* Tell the tx thread that the module is disconnected.
+ */
+
+ g_btdev.tx_status = ENOTCONN;
+
+ /* Make sure the thread is not blocked by the semaphore.
+ */
+
+ nxsem_post(&g_btdev.ncmd_sem);
+
+ /* We create and push a packet into the tx queue to unblock the thread.
+ */
+
+ buf = bt_hci_cmd_create(0xf00f, 0);
Review Comment:
What 0xf00f means? It is better to use a defined symbol instead of a hard-code value
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1444,6 +1453,7 @@ static void cmd_queue_init(void)
nxsem_init(&g_btdev.ncmd_sem, 0, 1);
g_btdev.ncmd = 1;
+ g_btdev.tx_status = 0;
Review Comment:
```suggestion
g_btdev.tx_status = OK;
##########
include/nuttx/wireless/bluetooth/bt_driver.h:
##########
@@ -119,4 +119,21 @@ struct bt_driver_s
int bt_netdev_register(FAR struct bt_driver_s *btdev);
+/****************************************************************************
+ * Name: bt_netdev_unregister
+ *
+ * Description:
+ * Unregister a network a driver registered by bt_netdev_register.
Review Comment:
```suggestion
* Unregister a network driver registered by bt_netdev_register.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] ldube commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1536366609
@acassis No. I pushed several changes after creating the pull request. I am in sync with ldube:master and 839109f482ea84b9c5c79d7aa141822e11f05e9a builds without issues on my machine.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] ldube commented on a diff in pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on code in PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#discussion_r1184152211
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1575,6 +1593,53 @@ int bt_driver_register(FAR struct bt_driver_s *btdev)
void bt_driver_unregister(FAR struct bt_driver_s *btdev)
{
+ int ret;
+ FAR struct bt_buf_s *buf;
+
+ /* Tell the tx thread that the module is disconnected.
+ */
+
+ g_btdev.tx_status = ENOTCONN;
+
+ /* Make sure the thread is not blocked by the semaphore.
+ */
+
+ nxsem_post(&g_btdev.ncmd_sem);
+
+ /* We create and push a packet into the tx queue to unblock the thread.
+ */
+
+ buf = bt_hci_cmd_create(0xf00f, 0);
Review Comment:
Nothing. Any packet will do but I will change it to a reset opcode. The tx thread will not transmit at this point.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on a diff in pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#discussion_r1184113449
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1575,6 +1593,53 @@ int bt_driver_register(FAR struct bt_driver_s *btdev)
void bt_driver_unregister(FAR struct bt_driver_s *btdev)
{
+ int ret;
+ FAR struct bt_buf_s *buf;
+
+ /* Tell the tx thread that the module is disconnected.
+ */
+
+ g_btdev.tx_status = ENOTCONN;
+
+ /* Make sure the thread is not blocked by the semaphore.
+ */
+
+ nxsem_post(&g_btdev.ncmd_sem);
+
+ /* We create and push a packet into the tx queue to unblock the thread.
+ */
+
+ buf = bt_hci_cmd_create(0xf00f, 0);
Review Comment:
What does 0xf00f means? It is better to use a defined symbol instead of a hard-code value
##########
wireless/bluetooth/bt_hcicore.c:
##########
@@ -1575,6 +1593,53 @@ int bt_driver_register(FAR struct bt_driver_s *btdev)
void bt_driver_unregister(FAR struct bt_driver_s *btdev)
{
+ int ret;
+ FAR struct bt_buf_s *buf;
+
+ /* Tell the tx thread that the module is disconnected.
+ */
+
+ g_btdev.tx_status = ENOTCONN;
+
+ /* Make sure the thread is not blocked by the semaphore.
+ */
+
+ nxsem_post(&g_btdev.ncmd_sem);
+
+ /* We create and push a packet into the tx queue to unblock the thread.
+ */
+
+ buf = bt_hci_cmd_create(0xf00f, 0);
Review Comment:
What does 0xf00f mean? It is better to use a defined symbol instead of a hard-code value
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] ldube commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1536420835
Confirmed. Renaming locally generates the same error.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #9166: wireless/bluetooth: Support removable bluetooth modules.
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #9166:
URL: https://github.com/apache/nuttx/pull/9166#issuecomment-1536224866
@ldube did you see these errors?
```
bluetooth/bt_hcicore.c: In function 'hci_tx_kthread':
Error: bluetooth/bt_hcicore.c:988:17: error: 'struct bt_dev_s' has no member named 'tx_status'
988 | for (; g_btdev.tx_status == OK; )
| ^
Error: bluetooth/bt_hcicore.c:1021:18: error: 'struct bt_dev_s' has no member named 'tx_status'
1021 | if (g_btdev.tx_status == OK)
| ^
Error: bluetooth/bt_hcicore.c:1036:10: error: 'struct bt_dev_s' has no member named 'tx_status'
1036 | g_btdev.tx_status = ESHUTDOWN;
| ^
bluetooth/bt_hcicore.c: In function 'cmd_queue_init':
Error: bluetooth/bt_hcicore.c:1456:10: error: 'struct bt_dev_s' has no member named 'tx_status'
1456 | g_btdev.tx_status = OK;
| ^
bluetooth/bt_hcicore.c: In function 'cmd_queue_deinit':
Error: bluetooth/bt_hcicore.c:1486:10: error: 'struct bt_dev_s' has no member named 'tx_status'
1486 | g_btdev.tx_status = ENOTCONN;
| ^
Error: bluetooth/bt_hcicore.c:1497:9: error: implicit declaration of function 'bt_hci_cmd_create' [-Werror=implicit-function-declaration]
1497 | buf = bt_hci_cmd_create(BT_HCI_OP_RESET, 0);
| ^~~~~~~~~~~~~~~~~
Error: bluetooth/bt_hcicore.c:1497:7: error: assignment to 'struct bt_buf_s *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1497 | buf = bt_hci_cmd_create(BT_HCI_OP_RESET, 0);
| ^
Error: bluetooth/bt_hcicore.c:1509:17: error: 'struct bt_dev_s' has no member named 'tx_status'
1509 | while (g_btdev.tx_status == ENOTCONN)
| ^
bluetooth/bt_hcicore.c: In function 'bt_initialize':
Error: bluetooth/bt_hcicore.c:1568:3: error: 'g_callback_list' undeclared (first use in this function)
1568 | g_callback_list = NULL;
| ^~~~~~~~~~~~~~~
bluetooth/bt_hcicore.c:1568:3: note: each undeclared identifier is reported only once for each function it appears in
Error: bluetooth/bt_hcicore.c:1569:3: error: 'g_scan_dev_found_cb' undeclared (first use in this function)
1569 | g_scan_dev_found_cb = NULL;
| ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org