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

chip-device-ctrl: do not hardcode UDP listen port #9309

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

bluebin14
Copy link
Contributor

Problem

chip-device-ctrl hardcodes the UDP listen port to CHIP_PORT+1. This makes it impossible to have more than one instance of the python controller on the same host. The target device is unable to tell which one is which and messages arrive to both which results in communication failure.

Change overview

Do not hardcode the UDP listen port.

Testing

How was this tested? (at least one bullet point required)

  • tested manually, running 2 simultaneous instances of python controller, performing full commissioning with each (PASE + CASE) followed by read/write commands to make sure everything works. This was not possible to do before this change.

Notes

A similar fix can be applied to chip-tool but that hasn't been tested yet so it is not included here.

@github-actions
Copy link

Size increase report for "esp32-example-build" from 51c9327

File Section File VM
chip-temperature-measurement-app.elf .flash.text -60 -60
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
[Unmapped],0,60
.flash.text,-60,-60

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize


@jmeg-sfy
Copy link
Contributor

What happens with a zero as parameter then, it s dynamically choose a port to listen to..?

@bluebin14
Copy link
Contributor Author

Yes, zero means use any (dynamic) port. This is a standard Unix convention.

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

This is OK as far as it goes, but only because we never end up resolving the controller's mdns advertisement so far. If we did, it would break, because the mdns advertisement always advertises CHIP_PORT.

Of course that was broken before this change too....

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