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

Add MicroPython support for VEDirect driver #95

Merged
merged 6 commits into from Sep 17, 2021

Conversation

nznobody
Copy link
Contributor

@nznobody nznobody commented Sep 2, 2021

Hi there,

based on my fork of @karioja's Python library for decoding the Victron Energy VE.Direct text protocol [1] (thanks a stack [2]), this patch will add support for MicroPython. On top of that, it will now understand a boolean async setting parameter, which will divert the actual sensor reading to a dedicated thread.

Cheers,
Manu.

[1] https://github.com/nznobody/vedirect
[2] Also thanks to @jmfife, who took over maintenance, added significant improvements and published the library on PyPI [3].
[3] https://pypi.org/project/vedirect-jmfife/

PS: I ran black over the file, it changed the quotes. Shall I revert those?

@amotl
Copy link
Member

amotl commented Sep 14, 2021

Dear Manu,

first of all, thanks a bunch for your excellent work on adding support for MicroPython to the VEDirect library. It is sweet that people will now be able to use it on microcontrollers running MicroPython.

With kind regards,
Andreas.

PS: I ran black over the file, it changed the quotes. Shall I revert those?

It is fine, no worries.

@amotl amotl changed the title Micropython support for VEDirect & threaded reader Add MicroPython support for VEDirect driver & multi-threading support Sep 14, 2021
@amotl
Copy link
Member

amotl commented Sep 14, 2021

With commit 5dd6da3, you said "add threading async reader. Not recommended for use probably... ". Do you still believe we should bring this in, or shall we just add your contribution without this commit?

@nznobody
Copy link
Contributor Author

nznobody commented Sep 14, 2021

Hello Andreas,

Thanks so much for taking the time to add CI testing, this will definitely make adding to this repository much more streamlined in the future :)

Also, great that you are such an active maintainer and working to integrate my changes 😄

With specific regard to commit 5dd6da3: I have tested it and it appears functional on a Fipy; however I am not sure about other Micropython and it is not well protected against errors in CPython. Possibly, if this feature is desired, I should polish it off a little. Additionally I have not researched the effects of threading on sleeping and power consumption.

I also did not use a lock and am unsure how threading will handle this in micropython. Since micropython lacks a more full threading library, I have not used it much.

Do you think it is worthwhile having threading? My rationale originally was because the VE devices often simply send out their values once every 10 or so seconds. That means if you synchronously wait for values you could be blocked for upto 10 seconds. I hope to read two VE devices and that means upto 20 seconds just waiting. Threading and storing the values resolves this.

I also have a minor change in there to insert the device product id (PID) into the variable name: "vedirect-{}:{}".format(product_id, key). This was to allow reading a couple different VE devices at once. However, upon reflection it may be better to use a name provided from the settings.py since people may want to read two identical products (e.g. two MPPT controllers).

Do you think it is worthwhile me tidying up threading or removing it with these insights?

Thanks again for your time,
Manu

@amotl
Copy link
Member

amotl commented Sep 14, 2021

Do you think it is worthwhile having threading? My rationale originally was because the VE devices often simply send out their values once every 10 or so seconds. That means if you synchronously wait for values you could be blocked for upto 10 seconds. I hope to read two VE devices and that means upto 20 seconds just waiting. Threading and storing the values resolves this.

Thanks for adding this explanation. It perfectly makes sense in this case. If there is really no way to send a signal to request a reading, I also don't see any other way to work around blocking the main thread.

I also have a minor change in there to insert the device product id (PID) into the variable name: "vedirect-{}:{}".format(product_id, key). This was to allow reading a couple different VE devices at once. However, upon reflection it may be better to use a name provided from the settings.py since people may want to read two identical products (e.g. two MPPT controllers).

When the current flavor of parameter-naming might lead to a collision, I would welcome amending the logic to be able to read more than one device of the same kind, yes.

@amotl
Copy link
Member

amotl commented Sep 14, 2021

Hi again,

That means if you synchronously wait for values you could be blocked for upto 10 seconds.

If there is really no way to send a signal to request a reading, I also don't see any other way to work around blocking the main thread.

I am not into the details of the VEDirect protocol at all, so please have mercy. On a quick research, I found this on page 4 of [1]:

Message format

The device transmits blocks of data at 1 second intervals.

and that at [2]:

How often you send a HEX message doesn't matter: there is no minimum. But beware, after receiving a HEX message, the Victron product will stop sending its usual TEXT frames for a few seconds. Therefor, if you keep sending HEX messages, you'll never receive the TEXT frame any more. Which might matter in case you are depending on data in the TEXT frame.

Might this be even related to your observations in any way?

With kind regards,
Andreas.

[1] https://www.sv-zanshin.com/r/manuals/victron-ve-direct-protocol.pdf
[2] https://www.victronenergy.com/live/vedirect_protocol:faq#q7how_about_frequency_of_transmitting_hex_messages

@nznobody
Copy link
Contributor Author

Hello Andreas,

Having reviewed your material: I agree that threading will not be required. A 1 second wait seems acceptable. You are correct that longer periods were seen with HEX messages. However, they will not be likely in this scenario. I will roll back the threading changes, but leave in the name collision avoidance. I might not get time to do it this week and am away next week, but will do it when I return.

Thanks again for your rigorous assistance with these merge requests.
Kind regards,
Manu

@amotl amotl force-pushed the feature/vedirect-micropython-async branch from 5dd6da3 to c6a494d Compare September 16, 2021 09:51
@amotl
Copy link
Member

amotl commented Sep 16, 2021

Dear Manu,

thank you for your response. I got some minutes of spare time, removed 5dd6da3 but added c6a494d instead. If in the future we will need the async/threading feature, I am open to bring it in again.

With kind regards,
Andreas.

@amotl amotl force-pushed the feature/vedirect-micropython-async branch from 82c20bc to ae72085 Compare September 16, 2021 10:23
@amotl
Copy link
Member

amotl commented Sep 16, 2021

ae72085 also fixes the installation of the improved VEDirect driver. Because the dependency_links arg was dropped by pip since v19 (see also [1]), there is no way to add custom / Git-repository-based Python modules to the official install_requires section in any way. So, I decided to put it into an "extras" section.

In this matter, the package can either be installed in development mode like python setup.py develop or, when using pip, like pip install --editable=.[vedirect] --find-links="https://github.com/nznobody/vedirect/tarball/345a688#egg=vedirect-2.0.0".

Without this amendment, it would break a vanilla, non-development installation attempt from PyPI using pip install terkin.

[1] https://stackoverflow.com/questions/64482089/python-setup-py-dependency-as-url-to-tar-or-git

@amotl amotl force-pushed the feature/vedirect-micropython-async branch from ae72085 to 9dda52e Compare September 16, 2021 10:32
@amotl amotl force-pushed the feature/vedirect-micropython-async branch from 9dda52e to 6cb0d5d Compare September 16, 2021 10:32
@amotl amotl changed the title Add MicroPython support for VEDirect driver & multi-threading support Add MicroPython support for VEDirect driver Sep 16, 2021
@amotl
Copy link
Member

amotl commented Sep 17, 2021

Dear Manu,

with 013ce38, I added a corresponding software test based on some testing infrastructure I found in the upstream vedirect repository. Kudos again to @karioja, @jmfife and you.

With kind regards,
Andreas.

@amotl amotl merged commit 90b0e27 into hiveeyes:master Sep 17, 2021
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.

None yet

2 participants