You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2021/06/08 10:20:52 UTC
[GitHub] [mynewt-nimble] haukepetersen opened a new pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
haukepetersen opened a new pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987
For doing run-time link quality assessment I am in the need of the echo request-response procedure provided by the L2CAP signalling layer. As of now, this functionality is not exposed through any NimBLE APIs. This PR exposes this functionality by adding the `ble_l2cap_ping()` function to NimBLE's l2cap API.
The usage is straight forward, once a BLE connection is established, one has simply to call the `ble_l2cap_ping()`. By passing an optional callback function to that function, this callback is executed one the `ECHO_RSP` packet is received. Additionally, an optional user payload can be appended to the `ECHO_REQ` packet.
Regarding testing of this feature: so far I only integrated and tested it with RIOT by exposing it as a `ping` subcommand for the `nimble_netif` shell command (-> https://github.com/haukepetersen/RIOT/tree/add_nimble_scnetifping):
```
2021-06-08 12:14:35,823 # ble ping 0
> 2021-06-08 12:14:35,920 # ECHO_RSP, rtt:95 size:0
2021-06-08 12:14:35,936 # ble ping 0
> 2021-06-08 12:14:36,070 # ECHO_RSP, rtt:132 size:0
ble ping 0 moinsen
2021-06-08 12:14:44,246 # ble ping 0 moinsen
> 2021-06-08 12:14:44,321 # ECHO_RSP, rtt:73 size:7
```
Should we also add this to some of the NimBLE examples/test applications to ensure the feature is properly picked up by the NimBLE testing?
--
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] [mynewt-nimble] haukepetersen commented on pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#issuecomment-916753721
all green -> go
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r663673032
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm I think ble_l2cap_sig_tx is (or should) consume txom regardless of result
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#issuecomment-915899365
Any chance someone finds the time to look at this feature? It would be great to have this in master :-)
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#issuecomment-871433995
rebased and fixed a bug where the signalling command's mbuf was not released in case the command failed.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r664287340
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
For further verification, here the code of `ble_l2cap_sig_tx()`:
```c
int
ble_l2cap_sig_tx(uint16_t conn_handle, struct os_mbuf *txom)
{
struct ble_l2cap_chan *chan;
struct ble_hs_conn *conn;
int rc;
ble_hs_lock();
rc = ble_hs_misc_conn_chan_find_reqd(conn_handle, BLE_L2CAP_CID_SIG,
&conn, &chan);
if (rc == 0) {
rc = ble_l2cap_tx(conn, chan, txom);
}
ble_hs_unlock();
return rc;
}
```
As one can see, no freeing of `txom` in case `ble_hs_misc_conn_chan_find_reqd()` fails...
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#issuecomment-915985075
wow, that was quick, thanks! :-) Squashed as requested.
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r663946311
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
Nope, unfortunately it does not. I have been debugging some of my nodes stopping to scan properly, until I found out that the node's msys buffer simply run out of mbufs, caused by this function not freeing `txom` on 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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#issuecomment-894057571
- removed the quick fix for releasing the used mbuf on error in favor of #999
- added precompiler guards around the ping function mapping to include them only if `BLE_L2CAP_COC_MAX_NUM != 0` to prevent compile errors if L2CAP COCs are not used
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r663673032
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm I think ble_l2cap_sig_tx is (or should) consuming txom regardless of result
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r664324471
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm but then, ble_l2cap_tx will consume txom even on error
maybe ble_l2cap_sig_tx() should be updated to always consume txom, btw I think ble_sm_tx() has same issue
I'll try to review those code parts this week and propose some unified solution
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen merged pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen merged pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r664324471
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm but then, ble_l2cap_tx will consume txom even on error
maybe ble_l2cap_sig_tx() should be updated to always consume txom, btw I think ble_sm_tx() has same issue
I'll try to lo review those code parts this week and propose some unified solution
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r664324471
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm but then, ble_l2cap_tx will consume txom even on error
maybe ble_l2cap_sig_tx() should be updated to always consume txom, btw I think ble_sm_tx() has same issue
I'll try to lo review those code parts this week and propose some unified solution
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
hmm but then, ble_l2cap_tx will consume txom even on error
maybe ble_l2cap_sig_tx() should be updated to always consume txom, btw I think ble_sm_tx() has same issue
I'll try to review those code parts this week and propose some unified solution
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r663946311
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
Nope, unfortunately it does not. I have been debugging some of my nodes stopping to scan properly, until I found out that the node's msys buffer simply run out of mbufs, caused by this function not freeing `txom` on error...
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
For further verification, here the code of `ble_l2cap_sig_tx()`:
```c
int
ble_l2cap_sig_tx(uint16_t conn_handle, struct os_mbuf *txom)
{
struct ble_l2cap_chan *chan;
struct ble_hs_conn *conn;
int rc;
ble_hs_lock();
rc = ble_hs_misc_conn_chan_find_reqd(conn_handle, BLE_L2CAP_CID_SIG,
&conn, &chan);
if (rc == 0) {
rc = ble_l2cap_tx(conn, chan, txom);
}
ble_hs_unlock();
return rc;
}
```
As one can see, no freeing of `txom` in case `ble_hs_misc_conn_chan_find_reqd()` fails...
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
I completely agree, this makes sense to be fixed coherently. Debugging this kind of inconsistency is no fun :-)
Let me know where I can help!
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #987: l2cap: implement echo request-response procedure (l2cap ping)
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #987:
URL: https://github.com/apache/mynewt-nimble/pull/987#discussion_r664388271
##########
File path: nimble/host/src/ble_l2cap_sig.c
##########
@@ -1979,6 +1979,9 @@ ble_l2cap_sig_ping(uint16_t conn_handle, ble_l2cap_ping_fn cb,
rc = ble_l2cap_sig_tx(proc->conn_handle, txom);
+ if (rc != 0) {
Review comment:
I completely agree, this makes sense to be fixed coherently. Debugging this kind of inconsistency is no fun :-)
Let me know where I can help!
--
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@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org