-
Notifications
You must be signed in to change notification settings - Fork 8.1k
doc: add simple L2CAP server example #96896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
doc: add simple L2CAP server example #96896
Conversation
bt_l2cap_chan_ops (fixes zephyrproject-rtos#96494) Signed-off-by: Ritesh Kudkelwar <[email protected]>
Ready for Review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a sample under samples/bluetooth
instead which is CI tested and will not go stale. You can refer to the sample here. And hopefully be able to have directly reference to the code in the sample in this document.
There is already various bsim test covering this functionality, for instance: zephyr/tests/bsim/bluetooth/host/l2cap/send_on_connect/src/main_l2cap_send_on_connect.c Lines 128 to 141 in 0ab4091
|
I think what Vinayak meant was that a sample is better than just additional documentation, and that we should add a BSIM test of that sample to help ensure that it always works (we've had some samples in the past that stopped working due to changes in the stack, and where only build issue have been fixed). |
I think the intention of this PR is to show in documentation how to set up a new L2CAP server. The user complains about lack of documentation in this area. Sample code which is also used for testing isn't always easy to grasp. For example, In this case, short section in documentation is really helpful because it is ready for simple copy-pasting. What could be a compromise here (keeping doc part and implementing a sample with covering it in CI) is using zephyr/doc/connectivity/usb/device_next/usb_device.rst Lines 69 to 73 in 2d72d86
zephyr/samples/subsys/usb/common/sample_usbd_init.c Lines 24 to 33 in 2d72d86
|
Yes, this is what I want, have the documentation but reference to actual code blocks from samples that are being built in CI (not necessarily CI tested, but that will be great too) and which do not go stale over time. A reader can copy these code blocks as instructed by the documentation. |
Thank you all! I added a real example at samples/bluetooth/l2cap_server_simple and updated the docs to show the code with literalinclude. The sample shows how to use fixed space for each connection (it uses CONFIG_BT_MAX_CONN and bt_conn_index(conn)). The docs example is now copied from this file. I am new here and still learning, so please let me know if I missed something or can do better. I’m trying hard to help and get things right. |
@cvinayak Hi is this changes met with your expectation please let me know. |
I think what needs to be done to complete this task is:
I can recommend you to look at l2cap ecred generic babblesim test. The code in both samples should not be overcomplicated (and your current sample in the PR is actually concise). You can look at, for example, peripheral and central samples to see how much code you need for both sample. Regarding documentation, I suggest to use |
02920b0
to
afcd52a
Compare
@PavelVPV Ready for Review |
There's many CI issues that you need to fix though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license and copyright headers also need to be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to follow the template for samples
https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/templates/sample.tmpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not addressed - please use the template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing a readme altogether
afcd52a
to
0e6fb96
Compare
Ready for review @kartben |
Probably not :) have you seen my message earlier regarding CI failures? There's even more now, it looks like, so please address them |
Also don't resolve comments yourself especially when things.havent been addressed |
@ritesh006 this too needs to be addressed please |
sorry this won't happen again |
0e6fb96
to
8975e67
Compare
@@ -0,0 +1,180 @@ | |||
/* | |||
* Copyright (c) 2022-2024 Nordic Semiconductor ASA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritesh006 are you a Nordic employee? If this is your code it should be your copyright, or a generic clause
https://docs.zephyrproject.org/latest/contribute/guidelines.html#copyright-and-license-notices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't been applied correctly. Please take some time to read the links shared by reviewers as we are otherwise spending a lot of time on things that should be rather trivial. Thanks!
…ephyrproject-rtos#96494) Signed-off-by: Ritesh Kudkelwar <[email protected]>
8975e67
to
446aaab
Compare
@@ -0,0 +1,83 @@ | |||
/* | |||
* SPDX-License-Identifier: Apache-2.0 | |||
* Copyright The Zephyr Project Contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.zephyrproject.org/latest/contribute/guidelines.html#copyright-and-license-notices
Please fix everywhere
* Copyright The Zephyr Project Contributors | |
* SPDX-FileCopyrightText: Copyright The Zephyr Project Contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
@@ -0,0 +1,180 @@ | |||
/* | |||
* Copyright (c) 2022-2024 Nordic Semiconductor ASA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't been applied correctly. Please take some time to read the links shared by reviewers as we are otherwise spending a lot of time on things that should be rather trivial. Thanks!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed READMEs yet. Will do this after @kartben comments are resolved there.
|
||
cmake_minimum_required(VERSION 3.20.0) | ||
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
project(l2cap_client_simple) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated
CONFIG_BT_SMP=y | ||
CONFIG_BT_L2CAP_DYNAMIC_CHANNEL=y | ||
# Optional, adjust for sample: | ||
CONFIG_BT_MAX_CONN=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as here: #96896 (comment)
|
||
cmake_minimum_required(VERSION 3.20.0) | ||
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
project(l2cap_server_simple) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated
.. literalinclude:: ../../../../samples/bluetooth/l2cap_coc_acceptor/src/main.c | ||
:language: c | ||
:linenos: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should point to exact lines not to the entire file. What should be demonstrated (based on your previous change) are line 17-48 from l2cap_coc_acceptor/src/main.c.
I also suggest to move the literalinclude
up to line 34 (right after paragraph that starts at line 28) and remove the existing lines 34-35 (Creating a simple L2CAP server...
).
Then paragraph at line 37 that refers to samples passes perfectly to the entire section. But! I suggest to keep only reference to acceptor (server) role there and move the initiator (client) reference to the end of Client Channels
section that you have added (after line 97 at the current change).
.. note:: | ||
The sample demonstrates allocating one channel per connection using | ||
``CONFIG_BT_MAX_CONN`` and ``bt_conn_index(conn)``. See the initiator sample | ||
for how to open a channel and send data to the acceptor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this note doesn't belong to l2cap.rst
but to README of the particular sample.
``CONFIG_BT_MAX_CONN`` and ``bt_conn_index(conn)``. See the initiator sample | ||
for how to open a channel and send data to the acceptor. | ||
|
||
Fixed Channels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change, thanks!
Adds a "Creating a simple L2CAP server" section to the L2CAP API docs with
a minimal skeleton example and step-by-step instructions.
Also fixes minor typos (enabled/latter).
Fixes: #96494