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

Use environment variables to configure the ports used by chip-tool #3675

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

vivien-apple
Copy link
Contributor

Problem

chip-tool ask the user to configure the port to send messages. This default probably does not need to change much.

Also in some situations it is useful to configure the port used by chip-tool to listen messages. For example if you run both the chip-tool client application and an instance of the chip-app-server on the same machine.

If you do not configure them, both try to listen on the same port, and so chip-tool sends messages to itself...

Summary of Changes

  • Add CHIP_TOOL_REMOTE_PORT environment variable
  • Add CHIP_TOOL_LOCAL_PORT environment variable
  • Rename CHIP_LOG_PROGRESS to CHIP_TOOL_LOG_PROGRESS
  • Do not ask anymore the chip-tool users to type in the port if they use the default one.
  • Update the documentation in case someone needs to override ports.

@mspang
Copy link
Contributor

mspang commented Nov 5, 2020

Can we use command line options instead?

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Would prefer not to use environment variables for this as it's a support burden (it sweeps the option under the rug, where it can affect unintended processes - say you run more than one chip-tool as part of a test).

@gerickson
Copy link
Contributor

Would prefer not to use environment variables for this as it's a support burden (it sweeps the option under the rug, where it can affect unintended processes - say you run more than one chip-tool as part of a test).

+1

@vivien-apple
Copy link
Contributor Author

Would prefer not to use environment variables for this as it's a support burden (it sweeps the option under the rug, where it can affect unintended processes - say you run more than one chip-tool as part of a test).

This case is fine if you use the env variable on the same line as the command. The scope of the variable will not affect the shell. For example CHIP_TOOL_REMOTE_PORT=11098 chip-tool onoff on ::1 1 will run without changing the value of the remote port for following commands.

That said, I understand your point and a command line options would be way much better. The real issue is that chip-tool has grown without true command line parsing facility and is a bit dumb at the moment when it comes to command arguments...

@mspang Would you be fine with a followup to fix the real issue. It may take some time to reformat chip-tool to support it and I would like to move forward in the meantime.

@rwalker-apple
Copy link
Contributor

Would prefer not to use environment variables for this as it's a support burden (it sweeps the option under the rug, where it can affect unintended processes - say you run more than one chip-tool as part of a test).

+1

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.

approving with the assumption that this will get a real fix soon

@mspang
Copy link
Contributor

mspang commented Nov 19, 2020

Would prefer not to use environment variables for this as it's a support burden (it sweeps the option under the rug, where it can affect unintended processes - say you run more than one chip-tool as part of a test).

This case is fine if you use the env variable on the same line as the command. The scope of the variable will not affect the shell. For example CHIP_TOOL_REMOTE_PORT=11098 chip-tool onoff on ::1 1 will run without changing the value of the remote port for following commands.

That said, I understand your point and a command line options would be way much better. The real issue is that chip-tool has grown without true command line parsing facility and is a bit dumb at the moment when it comes to command arguments...

@mspang Would you be fine with a followup to fix the real issue. It may take some time to reformat chip-tool to support it and I would like to move forward in the meantime.

Ok (Sorry - please use the re-request review button).

@mspang mspang self-requested a review November 19, 2020 20:49
@vivien-apple
Copy link
Contributor Author

Thanks.

I will rebase this one once I'm done converting chip-tool to the new DeviceController API. Since it has landed the code to customise the udp listen port seems to not work anymore and I don't want to spend time fixing the deprecated API.

@andy31415
Copy link
Contributor

@vivien-apple - any chance you could rebase? trying to cleanup old PRs and either get them merged or closed. Thanks!

@BroderickCarlin
Copy link
Contributor

@vivien-apple once the merge conflict is addressed it should be good to merge :D

@vivien-apple
Copy link
Contributor Author

I have rebased chip-tool over the new ChipDeviceController API in #3979 and I will rebase this one on top of it once #3979 has landed.

@vivien-apple
Copy link
Contributor Author

@vivien-apple - any chance you could rebase? trying to cleanup old PRs and either get them merged or closed. Thanks!

Sadly I need to rebase chip-tool over the new ChipDeviceController API before rebasing this one. The old API code to set the UDP listen port seems broken and I rather prefer spending time fixing the new API than the old one !

@rwalker-apple
Copy link
Contributor

@vivien-apple conflicts?

@vivien-apple vivien-apple force-pushed the ChipTool_ConfigurePorts branch from 2ec0e8c to 519ef53 Compare December 3, 2020 13:45
@vivien-apple
Copy link
Contributor Author

I finally rebased this PR on top of chip-tool with the new controller API ChipDeviceController and on top of #4071.

It will likely not build until #4071 lands though since I use the SetUdpListenPort API from it.

That said, I remove the needs for ENV variables and use the .ini file that chip-tool now uses to permanently store it's association with devices.

@vivien-apple vivien-apple force-pushed the ChipTool_ConfigurePorts branch from 519ef53 to ab29b4b Compare December 10, 2020 08:48
@andy31415 andy31415 merged commit ccbffc1 into project-chip:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants