Skip to content
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.

Sign up for GitHub

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

Jump to bottom

subsys: bluetooth: User data in HID service callback #19692

Merged
merged 1 commit into from
Jan 8, 2025
Merged

subsys: bluetooth: User data in HID service callback #19692

merged 1 commit into from
Jan 8, 2025

Conversation

pdunaj
Copy link
Contributor

@pdunaj pdunaj commented Dec 20, 2024

Allow user to set argument with which notification completion callback is called.

@pdunaj pdunaj requested review from a team as code owners December 20, 2024 13:45
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 20, 2024
@pdunaj
Copy link
Contributor Author

pdunaj commented Dec 20, 2024

why @nrfconnect/ncs-dragoon is needed to review this? @grochu , likely we need to update the co file

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Dec 20, 2024 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 2

Inputs:

Sources:

sdk-nrf: PR head: 6f92fad33559039433408ad6cd0215f84ef56414

more details

sdk-nrf:

PR head: 6f92fad33559039433408ad6cd0215f84ef56414
merge base: 3665a25025dcfb35f78b1d383fc37444d8618037
target head (main): 37fba0b31a04db7499cc9cc7c77f4e60a7b77117
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (2) 
include
│  ├── bluetooth
│  │  ├── services
│  │  │  │ hids.h
subsys
│  ├── bluetooth
│  │  ├── services
│  │  │  │ hids.c

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 853
  • ✅ Integration tests
    • ✅ desktop52_verification
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples
    • ✅ test-sdk-find-my
Disabled integration tests
    • doc-internal
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@pdunaj
bluetooth: services: User data in HID service callback
6f92fad 
Allow user to set argument with which notification completion
callback is called.

Signed-off-by: Pawel Dunaj <[email protected]>
@pdunaj pdunaj requested a review from MarekPieta December 20, 2024 15:00
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

MarekPieta
MarekPieta approved these changes Jan 2, 2025
View reviewed changes
include/bluetooth/services/hids.h
* @return 0 If the operation was successful. Otherwise, a (negative) error
* code is returned.
*/
int bt_hids_inp_rep_send_userdata(struct bt_hids *hids_obj, struct bt_conn *conn,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I would consider deprecating/removing the old approach to keep API simpler.
We may use a Kconfig to toggle between APIs (the Kconfig could be deprecated and old API would be removed at some point). We could also consider simply breaking the API for NCS 3.0 if it's acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a release note (API change)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave the old api as is to avoid generating noise. Note that if you deprecate the old function people will need to update their app and so waste time for no good reason.

I don't mind mentioning new function in release note but do we really intend to add a release note for any new api function if any random lib/service/module? Any opinion @b-gent ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not insist here. On the other hand, keeping API smaller/simpler e.g. makes it easier for new users to get started.

@rugeGerritsen
Copy link
Contributor

why @nrfconnect/ncs-dragoon is needed to review this? @grochu , likely we need to update the co file

@pdunaj , will you update the code owner file? That can be done in a separate PR

@MarekPieta MarekPieta mentioned this pull request Jan 2, 2025
Open
@pdunaj
Copy link
Contributor Author

pdunaj commented Jan 8, 2025

why @nrfconnect/ncs-dragoon is needed to review this? @grochu , likely we need to update the co file

@pdunaj , will you update the code owner file? That can be done in a separate PR

sure, I will create a PR to clean this up

@pdunaj
Copy link
Contributor Author

pdunaj commented Jan 8, 2025

@grochu , @maje-emb , approve or comment. I would prefer to merge it as is, don't see a reason to bother CI with rerun.

@pdunaj
Copy link
Contributor Author

pdunaj commented Jan 8, 2025

include/bluetooth/services/hids.h

#19793

@pdunaj pdunaj merged commit f39663d into nrfconnect:main Jan 8, 2025
13 checks passed
@pdunaj pdunaj deleted the hids_udcallback branch January 8, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarekPieta MarekPieta MarekPieta approved these changes

@maje-emb maje-emb maje-emb approved these changes

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pdunaj @NordicBuilder @rugeGerritsen @MarekPieta @maje-emb