samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample #84369

ramana-g-123 · 2025-01-22T10:47:17Z

Fixes #84187
The logic in central_multilink example outputs "Scanning successfully stopped" when the stop call has failed as the bt_le_scan_stop() call returns 0 when successful and error when unsuccessful.

github-actions · 2025-01-22T10:48:01Z

Hello @ramana-g-123, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

jhedberg · 2025-01-22T10:53:24Z

@ramana-g-123 thanks for the pull request! The change itself looks great, however please fix the commit message to be compliant with Zephyr requirements:

Add a prefix to the subject. This can be e.g. "samples: bluetooth: ..."
Remove the issue reference from the subject and instead have a short version of what the actual change is there, e.g. "samples: bluetooth: Fix bt_le_scan_stop() error checking"
Add a Signed-off-by line. git will do this automatically for you when you pass -s to git commit.
Add the issue reference to the PR description. GitHub recognizes this in the format "Fixes #...." and will link the two automatically together then.

sjanc · 2025-01-22T11:15:53Z

samples/bluetooth/central_multilink/src/central_multilink.c

@@ -75,7 +75,7 @@ static void device_found(const bt_addr_le_t *addr, int8_t rssi, uint8_t type,
 		return;
 	}

-	if (bt_le_scan_stop()) {
+	if (bt_le_scan_stop() == 0) {
 		printk("Scanning successfully stopped\n");


Hmm isn't the check correct but printed error message is wrong?

If bt_le_scan_stop returns 0 it means that it was stopped :) Looks right to me

Looking at the more general flow of the function, I believe the purpose is to proceed to the bt_conn_le_create() step when scanning was stopped, so we shouldn't return from the function in a success case. So it does seem like @sjanc is right, i.e. the log message is wrong. Since the function has an err variable, I'd also take advantage of it in the same go:

err = bt_le_scan_stop(); if (err != 0) { printk("Failed to stop scanning (err %d)\n", err); return; }

@ramana-g-123 can you update the PR accordingly? Thanks.

Agree with @sjanc and @jhedberg, on failure to stop scanning the control should not proceed further to setup a connection. Hence, it is the message that is incorrect.

@ramana-g-123 Thank you for noticing the issue.

jhedberg · 2025-01-22T12:43:44Z

samples/bluetooth/central_multilink/src/central_multilink.c

@@ -75,7 +75,7 @@ static void device_found(const bt_addr_le_t *addr, int8_t rssi, uint8_t type,
 		return;
 	}

-	if (bt_le_scan_stop()) {
+	if (bt_le_scan_stop() == 0) {
 		printk("Scanning successfully stopped\n");


Looking at the more general flow of the function, I believe the purpose is to proceed to the bt_conn_le_create() step when scanning was stopped, so we shouldn't return from the function in a success case. So it does seem like @sjanc is right, i.e. the log message is wrong. Since the function has an err variable, I'd also take advantage of it in the same go:

err = bt_le_scan_stop(); if (err != 0) { printk("Failed to stop scanning (err %d)\n", err); return; }

@ramana-g-123 can you update the PR accordingly? Thanks.

cvinayak · 2025-01-22T19:03:39Z

samples/bluetooth/central_multilink/src/central_multilink.c

@@ -75,7 +75,7 @@ static void device_found(const bt_addr_le_t *addr, int8_t rssi, uint8_t type,
 		return;
 	}

-	if (bt_le_scan_stop()) {
+	if (bt_le_scan_stop() == 0) {
 		printk("Scanning successfully stopped\n");


Agree with @sjanc and @jhedberg, on failure to stop scanning the control should not proceed further to setup a connection. Hence, it is the message that is incorrect.

@ramana-g-123 Thank you for noticing the issue.

cvinayak · 2025-01-23T05:12:27Z

@ramana-g-123 thanks for the pull request! The change itself looks great, however please fix the commit message to be compliant with Zephyr requirements:

* Add a prefix to the subject. This can be e.g. "samples: bluetooth: ..."

* Remove the issue reference from the subject and instead have a short version of what the actual change is there, e.g. "samples: bluetooth: Fix bt_le_scan_stop() error checking"

* Add a Signed-off-by line. git will do this automatically for you when you pass `-s` to `git commit`.

* Add the issue reference to the PR description. GitHub recognizes this in the format "Fixes #...." and will link the two automatically together then.

Code looks good now... @ramana-g-123 please follow above suggestion to have single commit in this PR.

jhedberg · 2025-01-23T09:57:50Z

@ramana-g-123 you still need a commit message body. You can find more info here: https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-message-guidelines

jhedberg · 2025-01-23T10:00:43Z

@ramana-g-123 and also note this failure: UC5 Commit title exceeds max length (84>75). FWIW, you can run the compliance script yourself prior to pushing, if you want to save some time: scripts/ci/check_compliance.py

jhedberg · 2025-01-27T07:11:02Z

@ramana-g-123 the commit message still isn't compliant, causing the Compliance check to fail: "Commit message body line exceeds max length (160>75)". Once that's fixed I think we can merge it.

jhedberg · 2025-01-27T07:26:28Z

@ramana-g-123 you don't need to close/re-open pull requests like this. Just a force-push is enough.

ramana-g-123 · 2025-01-27T08:00:41Z

Hey @jhedberg , sorry for the previous commit I was unable to run compliance_checks.py with the following issue even after installing all the packages needed using requirements.txt

  File "C:\Zephyr_RTOS\zephyr\scripts\ci\check_compliance.py", line 29, in <module>
    import magic
  File "C:\Zephyr_RTOS\zephyr\myenv\Lib\site-packages\magic\__init__.py", line 209, in <module>
    libmagic = loader.load_lib()
  File "C:\Zephyr_RTOS\zephyr\myenv\Lib\site-packages\magic\loader.py", line 49, in load_lib
    raise ImportError('failed to find libmagic.  Check your installation')
ImportError: failed to find libmagic.  Check your installation

Thalley · 2025-01-27T09:59:11Z

Hey @jhedberg , sorry for the previous commit I was unable to run compliance_checks.py with the following issue even after installing all the packages needed using requirements.txt

  File "C:\Zephyr_RTOS\zephyr\scripts\ci\check_compliance.py", line 29, in <module>
    import magic
  File "C:\Zephyr_RTOS\zephyr\myenv\Lib\site-packages\magic\__init__.py", line 209, in <module>
    libmagic = loader.load_lib()
  File "C:\Zephyr_RTOS\zephyr\myenv\Lib\site-packages\magic\loader.py", line 49, in load_lib
    raise ImportError('failed to find libmagic.  Check your installation')
ImportError: failed to find libmagic.  Check your installation

@ramana-g-123 did you install the requirements-compliance.txt as well?

ramana-g-123 · 2025-01-27T15:50:18Z

Hey @Thalley , I think requirements.txt has requirements-compliance.txt in it. And yeah I did install requirements-compliance.txt alone also and checked faced the same issue

Thalley · 2025-01-27T16:01:46Z

Hey @Thalley , I think requirements.txt has requirements-compliance.txt in it. And yeah I did install requirements-compliance.txt alone also and checked faced the same issue

Hmm, interestingly, it looks like the missing libmagic is not in the requirements.txt, but rather a lib from https://docs.zephyrproject.org/latest/develop/getting_started/index.html#install-dependencies. Not sure how well it works on Windows then. You could try https://pypi.org/project/python-libmagic/ - I wonder if we should defer to that package then, unless check_compliance.py won't run on Windows for other reasons.

@kartben any suggestions/input?

The issue that was pointed out has been fixed

Outputs error code to console if bt_le_scan_stop() fails Signed-off-by: Ramana Gudipudi <[email protected]>

github-actions · 2025-01-29T06:08:55Z

Hi @ramana-g-123!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added size: XS A PR changing only a single line of code area: Samples Samples area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Jan 22, 2025

zephyrbot requested review from alwa-nordic, cvinayak, hermabe, jhedberg, kartben, nashif, rugeGerritsen, sjanc, Thalley and theob-pro January 22, 2025 10:47

zephyrbot assigned jhedberg and alwa-nordic Jan 22, 2025

rugeGerritsen previously approved these changes Jan 22, 2025

View reviewed changes

sjanc reviewed Jan 22, 2025

View reviewed changes

jhedberg requested changes Jan 22, 2025

View reviewed changes

cvinayak previously requested changes Jan 22, 2025

View reviewed changes

ramana-g-123 dismissed rugeGerritsen’s stale review via 1dbfb6b January 23, 2025 04:48

ramana-g-123 changed the title ~~Update central_multilink.c to fix #84187~~ samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample Jan 23, 2025

ramana-g-123 closed this Jan 23, 2025

ramana-g-123 force-pushed the main branch from 1dbfb6b to 020cb79 Compare January 23, 2025 09:09

ramana-g-123 reopened this Jan 23, 2025

alwa-nordic previously approved these changes Jan 23, 2025

View reviewed changes

ramana-g-123 closed this Jan 26, 2025

ramana-g-123 force-pushed the main branch from 06fd8a4 to f0d0264 Compare January 26, 2025 12:06

ramana-g-123 reopened this Jan 26, 2025

ramana-g-123 dismissed alwa-nordic’s stale review via 83ccd34 January 26, 2025 12:09

Thalley previously approved these changes Jan 27, 2025

View reviewed changes

ramana-g-123 closed this Jan 27, 2025

ramana-g-123 force-pushed the main branch from 83ccd34 to 3585c37 Compare January 27, 2025 07:21

ramana-g-123 reopened this Jan 27, 2025

ramana-g-123 dismissed Thalley’s stale review via d93e3e6 January 27, 2025 07:54

jhedberg approved these changes Jan 27, 2025

View reviewed changes

Thalley approved these changes Jan 27, 2025

View reviewed changes

Thalley requested review from rugeGerritsen, alwa-nordic, cvinayak and sjanc January 27, 2025 08:39

rugeGerritsen approved these changes Jan 27, 2025

View reviewed changes

alwa-nordic approved these changes Jan 27, 2025

View reviewed changes

samples: bluetooth: Fix scan stop error in central_multilink sample

77571a4

Outputs error code to console if bt_le_scan_stop() fails Signed-off-by: Ramana Gudipudi <[email protected]>

kartben force-pushed the main branch from d93e3e6 to 77571a4 Compare January 28, 2025 16:46

kartben merged commit 93946dc into zephyrproject-rtos:main Jan 29, 2025
20 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample #84369

samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample #84369

ramana-g-123 commented Jan 22, 2025 •

edited by kartben

Loading

github-actions bot commented Jan 22, 2025

jhedberg commented Jan 22, 2025

sjanc Jan 22, 2025

Thalley Jan 22, 2025

jhedberg Jan 22, 2025

cvinayak Jan 22, 2025

jhedberg Jan 22, 2025

cvinayak Jan 22, 2025

cvinayak commented Jan 23, 2025

jhedberg commented Jan 23, 2025

jhedberg commented Jan 23, 2025

jhedberg commented Jan 27, 2025

jhedberg commented Jan 27, 2025

ramana-g-123 commented Jan 27, 2025

Thalley commented Jan 27, 2025

ramana-g-123 commented Jan 27, 2025

Thalley commented Jan 27, 2025

github-actions bot commented Jan 29, 2025

samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample #84369

samples: bluetooth: Fix bt_le_scan_stop() error checking in central_multilink sample #84369

Conversation

ramana-g-123 commented Jan 22, 2025 • edited by kartben Loading

github-actions bot commented Jan 22, 2025

jhedberg commented Jan 22, 2025

sjanc Jan 22, 2025

Choose a reason for hiding this comment

Thalley Jan 22, 2025

Choose a reason for hiding this comment

jhedberg Jan 22, 2025

Choose a reason for hiding this comment

cvinayak Jan 22, 2025

Choose a reason for hiding this comment

jhedberg Jan 22, 2025

Choose a reason for hiding this comment

cvinayak Jan 22, 2025

Choose a reason for hiding this comment

cvinayak commented Jan 23, 2025

jhedberg commented Jan 23, 2025

jhedberg commented Jan 23, 2025

jhedberg commented Jan 27, 2025

jhedberg commented Jan 27, 2025

ramana-g-123 commented Jan 27, 2025

Thalley commented Jan 27, 2025

ramana-g-123 commented Jan 27, 2025

Thalley commented Jan 27, 2025

github-actions bot commented Jan 29, 2025

ramana-g-123 commented Jan 22, 2025 •

edited by kartben

Loading