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

allow sending raw commands to devices #413

Merged
merged 11 commits into from
May 18, 2021
Merged

Conversation

lnr0626
Copy link
Contributor

@lnr0626 lnr0626 commented May 17, 2021

Proposed change

This arose while debugging #411 (and messing around with the other commands I found in the command tables). Essentially I found it a lot easier to debug the actual commands when I had the ability to send raw insteon commands through mqtt

I don't think this is a common use case, and there's some cleanup that needs to happen if this were desired (tests, docs, etc); but am happy to work through those if this is would be desired

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
  • Code documentation was added where necessary

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated

@krkeegan
Copy link
Collaborator

I can understand how something like this would be helpful.

I think you should allow a full extended message to be defined, not just all zero bytes. That gets a bit unweildy from the command line, but from a json mqtt message, the extended bytes can just be a list.

@lnr0626
Copy link
Contributor Author

lnr0626 commented May 17, 2021

It does allow full extended commands - the mqtt message takes a data list, and the cli takes a -d/--data argument that's zero padded and trimmed to 14 bytes - the --ext-req flag is purely a convenience shorthand :-)

@krkeegan
Copy link
Collaborator

Ahh, I messed that. That is perfect then.

Yes, this would have been quite helpful at times. I had failed to realize it could be added so easily.

insteon_mqtt/device/base/Base.py Show resolved Hide resolved
insteon_mqtt/device/base/Base.py Outdated Show resolved Hide resolved
@lnr0626
Copy link
Contributor Author

lnr0626 commented May 18, 2021

Cool beans, I'll add some docs and tests to get this ready to merge.

I'll also move the pad/trim logic to the message handler, and may switch the cmd name to raw_command or raw - do you have a preference?

@krkeegan
Copy link
Collaborator

I don't have a name preference. I can see a value in either.

@lnr0626 lnr0626 marked this pull request as ready for review May 18, 2021 19:06
@lnr0626 lnr0626 closed this May 18, 2021
@lnr0626 lnr0626 deleted the mqtt-raw-commands branch May 18, 2021 19:20
@lnr0626 lnr0626 restored the mqtt-raw-commands branch May 18, 2021 19:20
@lnr0626 lnr0626 reopened this May 18, 2021
@krkeegan
Copy link
Collaborator

Looks good to me, unless you have any more changes I will merge.

@lnr0626
Copy link
Contributor Author

lnr0626 commented May 18, 2021

I think it's good to go, feel free to merge away :-)

@krkeegan krkeegan merged commit 0e3c6c7 into TD22057:dev May 18, 2021
@krkeegan
Copy link
Collaborator

This works in my setup. I didn't test extensively, but a basic test of standard commands worked as expected. Thank you for this, it is very helpful.

@lnr0626 lnr0626 deleted the mqtt-raw-commands branch May 18, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants