search

nugget / python-insteonplm

Python 3 asyncio module for interfacing with Insteon Powerline modems
MIT License
33 stars 19 forks source link

Removed quick_start parameter on plm and exposed load_aldb #101

Closed wz2b closed 6 years ago

wz2b commented 6 years ago

I removed the quick_start parameter on plm, and replaced it with load_aldb which was present at the PLM layer but not at Connection (where one creates a PLM). This exposed what was essentially the 'quick start' feature that doesn't appear to have been completed.

teharris1 commented 6 years ago

Thanks for looking into this. I saw your issues come in but have not had time to look at them.

wz2b commented 6 years ago

I'm happy to help - I want to be a contributor, not just a complainer. I also wanted to try to offer you a simple change to see how it goes, before I try to add in the new thermostat support.

I don't know anything about flake8 or pylint (other than what they are) but discovered I can run tox --develop and it it runs those things. I get a complaint about too many arguments to Connection.create() probably because I added one and pushed it over the limit. That's going to be hard to fix (especially without breaking backward compatibility) unless you can think of a good way to allow the user to


connection.setSomeOption()
connection.setSomeOtherOption()
connection.start()```

which would allow removal of some existing options if we find they aren't used.  If that's the strategy you want, then maybe `load_aldb=False` is not forward-thinking enough... I'm open to doing it either way, I definitely don't want to maintain my own fork of this indefinitely.  I forked it with the intent of being able to contribute back.

Other question: when I fix that problem you pointed out, do I submit a second pull request or is there some way to modify this one to request the pull from another commit?

On Thu, Sep 6, 2018 at 10:41 PM Tom Harris <notifications@github.com> wrote:

> Thanks for looking into this. I saw your issues come in but have not had
> time to look at them.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/nugget/python-insteonplm/pull/101#issuecomment-419303065>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABc2OVu14uOIc0bdx477-VdHjrc4Qstoks5uYdzdgaJpZM4WeEXb>
> .
>
wz2b commented 6 years ago

Since this is the first time I'm doing this I need a walk-through on a few pieces:

insteonplm/__init__.py:135:4: R0913: Too many arguments (11/10) (too-many-arguments)

------------------------------------
Your code has been rated at 10.00/10

ERROR: InvocationError for command '/home/chrisp/work/python-insteonplm/.tox/pylint/bin/pylint insteonplm tests' (exited with code 8)

The too many arguments is because pylint doesn't want more than 10, as I mentioned in the previous comment I'm not sure I can do anything about that without either more major changes, or risking breaking backward compatibility. But that InvocationError is something I couldn't figure out how to fix. pylint is there and the stated location, and it runs. Is this the problem?

SKIPPED: InterpreterNotFound: python3.6

teharris1 commented 6 years ago

When you commit your changes to your forked repository this PR will update with the new code. As for the lint issue of 'too-many-arguments' it is OK to explicitly tell the interpreter to ignore that issue with the following tag just above the method definition: # pylint: disable=too-many-arguments This allows us to find those later and fix them when it makes sense. But for now it just a matter of documenting them. At some point we will need to make breaking changes but not for each PR.

tox is the right way to run the tests since it also runs the pytest scripts as well. You can also link your repository to travis-ci which will run the tests each time you commit.

wz2b commented 6 years ago

When you commit your changes to your forked repository this PR will update with the new code.

Organizationally, does that mean I screwed up, and I shouldn't be issuing a PR on Master, but rather I should have created a feature branch or possibly an 'upstream to be committed' branch?

On Fri, Sep 7, 2018 at 9:15 AM Tom Harris notifications@github.com wrote:

When you commit your changes to your forked repository this PR will update with the new code. As for the lint issue of 'too-many-arguments' it is OK to explicitly tell the interpreter to ignore that issue with the following tag just above the method definition:

pylint: disable=too-many-arguments

This allows us to find those later and fix them when it makes sense. But for now it just a matter of documenting them. At some point we will need to make breaking changes but not for each PR.

tox is the right way to run the tests since it also runs the pytest scripts as well. You can also link your repository to travis-ci which will run the tests each time you commit.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/101#issuecomment-419435574, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2OWVwRSWdLIOkI9J4lcDMrQMVM7yDks5uYnF6gaJpZM4WeEXb .

teharris1 commented 6 years ago

Just to give you a little more background on the "fast start" option I was thinking about, there are a few concepts I was playing with but they were not high enough priority for me to finish the code. Here they are: 1) The library has to have a device in the IM.devices list in order to do anything. There are three ways to load that list. a. Load the ALDB and perform the IM.get_device_info process to identify the devices (this is the default way) b. Load the saved devices which already has the most recently known devices c. "Manually" add the devices based on some other external list.

  1. Option "c" has a lot of issues with it but probably still needs to be there. The issue is it runs the risk of a device being added that is not in the IM's ALDB. In that situation, the IM and the device communications will produce flaky results. (one is talking but the other is not listening.)
  2. If you load devices with option "b" that is very fast and no traffic. It is also reliable if it was generated via option "a".
  3. Option "a" is very reliable (except for batter operated devices that don't wake up, but that is why "device override" was created.). But it takes much longer (i.e. 5 - 15 secs per device)

So with all that background, the question is 'if you have a saved device file, i.e. option "b", do you just use it and skip option "a"?' The implication of this is if a device was added to the IM's ALDB you may not know about it until you reload the ALDB. The current start up process loads the saved devices then loads the ALDB, this was a trade off to allow the devices to load quickly but also to ensure that new devices were found. There are two downsides to this:

  1. After the devices are loaded there is a lot of traffic generated so the devices are useless anyway because the messages are sent in a serial mode. In other words, the devices cannot be manipulated until the ALDB laod process finishes anyway.
  2. If you remove a device from the IM's ALDB offline using another tool the device still shows up. Not as concerned about that one since this is outside our control and is fixed by simply deleting the insteon_plm_device_info.dat file.

The conclusion I came to is:

  1. Load the ALDB if there is no saved device file
  2. Allow the ALDB to be purged and reloaded so the user can "refresh" the ALDB when they make changes. this will then need to refresh the saved device file.
  3. Ensure that the All-Link process updates the ALDB so that if a device is added or removed via this library the IM's ALDB is in sync with that change. This will also need to refresh the saved device file.

I am not opposed to this PR since it does not break anything and allows a consumer of the library to decide how they want to populate IM.devices. So no worries there but wanted to give you a full picture of my thought process.

teharris1 commented 6 years ago

Organizationally, does that mean I screwed up, and I shouldn't be issuing a PR on Master, but rather I should have created a feature branch or possibly an 'upstream to be committed' branch?

Technically, no but now your repository has commits that will not really be in this master repository. The issue is more on your side now. Every time you post a new PR you will be reposting all of your prior commits unless you blow away your fork and start from a new clone. So the best practice for you is: 1) Fork this repository 2) Create a new branch 3) COmmit to the new branch and submit a PR agains this master branch from that branch 4) After the PR is approved/rejected, delete that branch 5) Go back to your master branch and merge with the upstream (i.e. this repository) master branch.

What this does is it allows you to have a forked master branch that always tracks this master. All your feature branches will come from your master branch. This way you don't have to blow away and recreate your repository each time you post a PR. Remember to merge your forked master with this repository every time you start a new feasture that way you are working against the most recent code base.

wz2b commented 6 years ago

That's great, thank you.

I think all the automatic stuff you're asking for makes sense, but I would like the ability to take control of these things myself:

  1. Connect to the PLM. Opens serial port, maybe makes sure PLM is there, but not much else.
  2. Configure things, set up listeners, maybe point it to the name of the .dat file and tell it to load it (if I want)
  3. Start discovery

In some cases I might not care about discovery at all. I might just want to launch a specific message at a device address. In some cases I can probably expand the model (for instance add a "Set LED Backlighting Mode" to the 2441V thermostat adapter).

Either way (automatic or not) I'm a little concerned about knowing the state of the IM. I can get a callback telling me it's done loading the ALDB but really the library is still busy after that discovering individual devices, and I'm not sure how to tell when it's complete. I would like to fix that.

Part of the reason I want a multi-step initialization (rather than kicking everything off in Connection.create()) is that I think there's a race condition in setting up that listener. I think in a practical sense it can't ever happen, because asyncio is doing the scheduling on a single loop and because things don't happen quickly enough but it still bothers me from a general principles perspective.

So I'm thinking of a couple of things now:

  1. Did my load_aldb go far enough or is more control needed
  2. Should aldb_refresh() include an option to NOT do individual device discovery until I ask
  3. Can I add a callback that notifies me when detailed device discovery is complete; and maybe another one that tells me progressively when devices have been discovered
  4. What's with the aldb load complete callback doing a pop() essentially clearing the list? That's a weird pub-sub pattern to use, it's a kind of "subscribe to one event only" that makes me pretty uncomfortable. If there's a reason it should be that way, can it at least be named something other than .add ........... I don't know, it just threw me for a loop when I encountered it.

Thanks for your patience with me on this. I'm a very experienced software engineer; but not much time in the python world.

On Fri, Sep 7, 2018 at 9:34 AM Tom Harris notifications@github.com wrote:

Just to give you a little more background on the "fast start" option I was thinking about, there are a few concepts I was playing with but they were not high enough priority for me to finish the code. Here they are:

  1. The library has to have a device in the IM.devices list in order to do anything. There are three ways to load that list. a. Load the ALDB and perform the IM.get_device_info process to identify the devices (this is the default way) b. Load the saved devices which already has the most recently known devices c. "Manually" add the devices based on some other external list.

  2. Option "c" has a lot of issues with it but probably still needs to be there. The issue is it runs the risk of a device being added that is not in the IM's ALDB. In that situation, the IM and the device communications will produce flaky results. (one is talking but the other is not listening.)

  3. If you load devices with option "b" that is very fast and no traffic. It is also reliable if it was generated via option "a".

  4. Option "a" is very reliable (except for batter operated devices that don't wake up, but that is why "device override" was created.). But it takes much longer (i.e. 5 - 15 secs per device)

So with all that background, the question is 'if you have a saved device file, i.e. option "b", do you just use it and skip option "a"?' The implication of this is if a device was added to the IM's ALDB you may not know about it until you reload the ALDB. The current start up process loads the saved devices then loads the ALDB, this was a trade off to allow the devices to load quickly but also to ensure that new devices were found. There are two downsides to this:

  1. After the devices are loaded there is a lot of traffic generated so the devices are useless anyway because the messages are sent in a serial mode. In other words, the devices cannot be manipulated until the ALDB laod process finishes anyway.
  2. If you remove a device from the IM's ALDB offline using another tool the device still shows up. Not as concerned about that one since this is outside our control and is fixed by simply deleting the insteon_plm_device_info.dat file.

The conclusion I came to is:

  1. Load the ALDB if there is no saved device file
  2. Allow the ALDB to be purged and reloaded so the user can "refresh" the ALDB when they make changes. this will then need to refresh the saved device file.
  3. Ensure that the All-Link process updates the ALDB so that if a device is added or removed via this library the IM's ALDB is in sync with that change. This will also need to refresh the saved device file.

I am not opposed to this PR since it does not break anything and allows a consumer of the library to decide how they want to populate IM.devices. So no worries there but wanted to give you a full picture of my thought process.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/101#issuecomment-419440665, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2Oed6W-2-CNt10utgXOWeEIcGnnZeks5uYnXNgaJpZM4WeEXb .