Increment transaction numbers and extract register function

[ianperrin]

Hi @GeoffAtHome

Great work, thank you.

I’ve done some refactoring and made a couple of modifications:

1. The transaction numbers sent to the link are now incremented in the range 1 to 999. This should avoid any conflicts when the commands are being interpreted by the link and the UDP responses are being parsed.

2. The register functionality has been extracted into its own function to allow clients to call this independently of other commands and a deregister_all function added

   If these can be pulled into the module so they are available via PyPi that would be great.

[GeoffAtHome]

Hi Ian,

Looks good - I need to carefully review the code as I am not great at python. Learnt something new - Cycle. I've not seen this before and I like what it does.

Hopefully I'll complete this tomorrow when I have more time and a clear head.

Cheers, Geoff

[ianperrin]

No worries, my clear head was today so good luck!

Ian

On 31 Dec 2018, at 18:13, GeoffAtHome notifications@github.com wrote:

Hi Ian,

Looks good - I need to carefully review the code as I am not great at python. Learnt something new - Cycle. I've not seen this before and I like what it does.

Hopefully I'll complete this tomorrow when I have more time and a clear head.

Cheers, Geoff

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[GeoffAtHome]

All good except the change 'Refactor instance variables and use of self'. What I am trying to do here is to have a single thread and single queue for all instances of LWLink().

the_queue, and thread are shared between all LWLink() instances hence not using self. In this implementation only a single LW hub is supported, hence moving link_ip into the class.

If multiple hubs need to be supported I would refactor this by wrapping the original implementation in another class. Each hub will have it's own IP address, thread and queue.

Perhaps this should be noted in the documentation and comments to avoid confusion.

[ianperrin]

Interesting idea, hadn’t thought of that use case.

What about moving the_queue and thread to global variables for now, then expanding as necessary later?

[GeoffAtHome]

I am currently using it with this use case. Where I have multiple instances of the class. One for lights and another for switches.

Moving the_queue, thread and link_ip into the scope of the class is better than globals, hence the original implementation.

[ianperrin]

Okay.

Well I can revert the change or leave as is - Let me know what you’d rather

In the long run, perhaps queuing and threading may be something which is handled by the calling application?

On 2 Jan 2019, at 19:09, GeoffAtHome notifications@github.com wrote:

I am currently using it with this use case. Where I have multiple instances of the class. One for lights and another for switches.

Moving the_queue, thread and link_ip into the scope of the class is better than globals, hence the original implementation.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[GeoffAtHome]

link_ip, transaction_id, the_queue and thread need to be made class variable. I.e. accessed via LWLink.thread etc and NOT self.thread

transaction_id needs to fall into the same camp to avoid clashes of transaction_id between instances of LWLink

[ianperrin]

Done

On 3 Jan 2019, at 09:49, GeoffAtHome notifications@github.com wrote:

link_ip, transaction_id, the_queue and thread

[GeoffAtHome]

Thanks - merged.