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

ESP32 - Update advertising data to be closer to the spec #1911

Merged
merged 1 commit into from
Jul 29, 2020

Conversation

vivien-apple
Copy link
Contributor

Problem

This PR update the ble advertising data to get it closer to the spec.

Summary of Changes

  • Update the advertising data on the esp32
  • Change the connection delegate to use the content of the advertising data instead of the connection name to connect to the peripheral
  • Change the code in examples/chip-tool/ to pass the discriminator instead of a name
  • Change the code in src/darwin to pass the discriminator instead of a name

@github-actions
Copy link

Size increase report for "gn_nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "gn_linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.out and ./pull_artifact/chip-nrf52840-lock-example.out:

sections,vmsize,filesize
.debug_str,0,7
.debug_info,0,-7
[Unmapped],0,-7
.debug_loc,0,-13


@github-actions
Copy link

Size increase report for "linux-example-build"

File Section File VM
chip-standalone-demo.out .text 208 208
chip-standalone-demo.out .dynsym 72 72
chip-standalone-demo.out .rela.plt 72 72
chip-standalone-demo.out .dynstr 71 71
chip-standalone-demo.out .plt 48 48
chip-standalone-demo.out .plt.sec 48 48
chip-standalone-demo.out .eh_frame 40 40
chip-standalone-demo.out .got 24 24
chip-standalone-demo.out .gnu.version 6 6
chip-standalone-demo.out [LOAD #2 [R]] -5 -5
chip-standalone-demo.out [LOAD #5 [RW]] -24 -24
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-standalone-demo.out and ./pull_artifact/chip-standalone-demo.out:

sections,vmsize,filesize
.debug_info,0,1591
.debug_str,0,1097
.debug_loc,0,1095
.debug_line,0,210
.text,208,208
.debug_ranges,0,160
.debug_abbrev,0,159
.dynsym,72,72
.rela.plt,72,72
.symtab,0,72
.dynstr,71,71
.strtab,0,57
.plt,48,48
.plt.sec,48,48
.eh_frame,40,40
.got,24,24
.gnu.version,6,6
[LOAD #2 [R]],-5,-5
[LOAD #5 [RW]],-24,-24
[Unmapped],0,-561


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow


@woody-apple
Copy link
Contributor

[self connect:peripheral];
[self stopScanning];
NSNumber * isConnectable = [advertisementData objectForKey:CBAdvertisementDataIsConnectable];
if ([isConnectable boolValue]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

supernit: how do people feel about early return and continues to decrease code indents? I find it easier to understand code if it is less indented once it starts growing, however I also see a pattern in other chip places of 'only one exit point' (but does that apply to non-resource constrained code?)

like:

if (![isConnectable boolValue]) {
  return;  // only connectable devices are useful
}

CBUUID * .....
for (....) {
  if (!... isEqualToData...) { 
     continue;
  }

  if (....) { 
  }
}

vs.

if ([isConnectable boolValue]) {
  CBUUID * .....;

  for (....) {
     if (...) { 
       if (...) {
       }
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my part I'm 100% in favour or early returns and to declared variables just before they are used, if possible.
I think @gerickson advocates for the 'single exit point' patterns thought.

advData[index++] = advDataVersionDiscriminator & 0x00FF;

// Construct the Chip BLE Service Data to be sent in the scan response packet.
err = esp_ble_gap_config_adv_data_raw(advData, sizeof(advData));
Copy link
Contributor

Choose a reason for hiding this comment

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

does the size always need to be sizeof == MAX_ADV_DATA_LEN or could we use index here to reduce payload sizes? does it matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point we will need to use manufacturer specific data and add them to the advertising data if there is enough space. The rest will go into the Scan Req/Resp packet.
I will dig into this when I will work on that in order to give you a more accurate answer.

@andy31415
Copy link
Contributor

Will merge since my comments were more nit/meh. Would love an answer, however if we need to adjust code we can do it so in a separate PR.

@andy31415 andy31415 merged commit 2b25999 into project-chip:master Jul 29, 2020
chirag-silabs pushed a commit to rosahay-silabs/connectedhomeip that referenced this pull request Jul 15, 2024
Merge in WMN_TOOLS/matter from fix/caracal_matter_templates to silabs_slc_1.3

Squashed commit of the following:

commit c7bfc56563f34eb152cdcdca3e6c15d4944c9fb3
Author: Sarthak Shaha <[email protected]>
Date:   Tue May 28 12:59:32 2024 -0400

    added missing space

commit e11a0a23281f07390ace70e23a8c47bbe9e61512
Author: Sarthak Shaha <[email protected]>
Date:   Tue May 28 12:31:24 2024 -0400

    revert

commit d849715b699bf2111cfd29f87b892095ecccd094
Author: Sarthak Shaha <[email protected]>
Date:   Tue May 28 12:29:03 2024 -0400

    adding appropriate boards/parts for caracle

... and 1 more commit
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.

5 participants