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/04/26 20:32:48 UTC
[GitHub] [nuttx] ldube opened a new pull request, #9113: wireless/bluetooth: Initialize private bt_driver_s member.
ldube opened a new pull request, #9113:
URL: https://github.com/apache/nuttx/pull/9113
## Summary
The priv member is currently unused but should not be null after the bt driver is registered.
## Impact
None
## Testing
Modified code and printed the value of priv. It is not NULL like it used to be.
--
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#issuecomment-1528690591
@ldube please fix the below nxstyle warning:
```
Error: /home/runner/work/nuttx/nuttx/nuttx/include/nuttx/wireless/bluetooth/bt_driver.h:88:52: error: Block comment terminator must be on a separate line
Error: /home/runner/work/nuttx/nuttx/nuttx/include/nuttx/wireless/bluetooth/bt_driver.h:93:40: error: Block comment terminator must be on a separate line
```
you can verify the fix locally:
./tools/checkpath.sh -g HEAD~...HEAD
--
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #9113:
URL: https://github.com/apache/nuttx/pull/9113
--
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179855742
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
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.
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179582682
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
Thanks for your input. Right now I will have to fork and finish my USB bluetooth project in a private branch. I need to make sure this stack will work with removable USB bluetooth modules before I commit any more time to this PR. If I feel the need to upstream my changes I will see if I can fix those drivers.
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.
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1178561491
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
but this field is used by driver self not the 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.
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1178665633
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
but many drivers already use this field to save it's context:(.
--
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179267324
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
This is clearly a private field that is reserved for the stack. My bt_netdev_unregister example above shows why the stack **needs** this field. The comment supports this use case. The correct way to use this "interface" is to create a "subclass".
struct my_driver {
bt_driver_s btdev;
void *priv;
}
To answer your question directly: No, we should not change the comment in bt_driver_s (bt_driver.h). I have given my reasons above. Any drivers abusing the bt_driver_s->priv field needs to be corrected. For other structs, the community can decide.
--
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1178623024
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
The driver should not use this field. The file bt_driver.h has this comment:
/* Filled by register function, shouldn't be touched by bt_driver_s */
I would like to create a bt_netdev_unregister(FAR struct bt_driver_s *btdev) function that does this:
{
FAR struct btnet_driver_s *priv;
priv = (FAR struct btnet_driver_s *)btdev->priv; <--------
netdev_unregister(&priv->bd_dev.r_dev);
}
--
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 #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "ldube (via GitHub)" <gi...@apache.org>.
ldube commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179267324
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
This is clearly a private field that is reserved for the stack. My bt_netdev_unregister example above shows why the stack **needs** this field. The comment supports this use case. The correct way to use this "interface" is to create a "subclass".
struct my_driver {
bt_driver_s btdev;
void *priv; <---- use this to save your drivers "context"
}
To answer your question directly: No, we should not change the comment in bt_driver_s (bt_driver.h). I have given my reasons above. Any drivers abusing the bt_driver_s->priv field needs to be corrected. For other structs, the community can decide.
--
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1178665633
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
but many drivers already use this field to save it's context:(, so should we update the comment to reflect the truth.
--
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179445730
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
If so, this patch needs to fix the drivers which use this field to save self context before it can be merged.
--
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 a diff in pull request #9113: wireless/bluetooth: Initialize private bt_driver_s member.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9113:
URL: https://github.com/apache/nuttx/pull/9113#discussion_r1179445730
##########
wireless/bluetooth/bt_netdev.c:
##########
@@ -1194,7 +1194,7 @@ int bt_netdev_register(FAR struct bt_driver_s *btdev)
/* Get the interface structure associated with this interface number. */
- priv = (FAR struct btnet_driver_s *)
+ btdev->priv = priv = (FAR struct btnet_driver_s *)
Review Comment:
If so, this patch needs to fix the drivers which use this field to save self context before it can be merged:
https://github.com/apache/nuttx/blob/master/drivers/serial/uart_bth4.c
https://github.com/apache/nuttx/blob/master/drivers/wireless/bluetooth/bt_bridge.c
https://github.com/apache/nuttx/blob/master/drivers/wireless/bluetooth/bt_rpmsghci_server.c
--
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