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.

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

[zephyr] Update BLE advertising data #2821

Merged
merged 2 commits into from
Sep 29, 2020

Conversation

Damian-Nordic
Copy link
Contributor

Problem

BLE advertising data on Zephyr was not aligned with the spec and other implementations like iOS & ESP32

Summary of Changes

Put all device identification data in the advertising payload (instead of scan response payload)
Set "Rendezvous over BLE" flag in QR Code in nRF Connect examples
By the way, deduplicate code for logging QR Code payload


#include <setup_payload/QRCodeSetupPayloadGenerator.h>

#include <logging/log.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

if this file used CHIP logging instead of nrf logging, this facility could be moved into CHIP?

Copy link
Contributor Author

@Damian-Nordic Damian-Nordic Sep 28, 2020

Choose a reason for hiding this comment

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

Since there's currently no application component within CHIP, by "moved into CHIP" you mean e.g. examples/common/chip-app-server?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I suggest moving this to src/setup_payload and including it in the library built there.

examples/common/ really only pertains to code that shouldn't end up in CHIP, like that used to deal with displays, etc.

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'm not sure.... by moving this code to setup_payload we would introduce a dependency between setup_payload and the platform library and I wonder if what we could potentially benefit from such a change is worth it. I would rather see code which glues underlying CHIP components together in the CHIP "application server" library which is to be created in #2927. What do you think?
For now I think I can remove non-essential parts of the PR which were only meant to address code duplication until #2927 is finished.

Copy link
Contributor

Choose a reason for hiding this comment

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

excellent points

Copy link
Contributor

@rwalker-apple rwalker-apple left a comment

Choose a reason for hiding this comment

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

suggest moving this into CHIP

@Damian-Nordic
Copy link
Contributor Author

suggest moving this into CHIP

Done!

* Also move QR Code printing code to examples/common to be
  shared across all examples.
* Restyled by whitespace
@rwalker-apple
Copy link
Contributor

suggest moving this into CHIP

Done!

shucks, I wish I'd been able to respond more quickly.

ultimately, chip-app-server should end up in src/app #2927

@rwalker-apple rwalker-apple merged commit 8dc7e7f into project-chip:master Sep 29, 2020
@Damian-Nordic Damian-Nordic deleted the feature/ble-adv branch October 15, 2020 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants