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 asyncio support #56

Open
wants to merge 121 commits into
base: develop
Choose a base branch
from

Conversation

GimmickNG
Copy link

With reference to my comment in #5, I've added asyncio support for TCP and limited support for Serial RTU. Right now, async serial support is TBD but I've left the class in just in case anyone else wants to try modifying it themselves. To avoid code duplication, I've based the async classes off their synchronous equivalents, and only modified parts when I couldn't cleanly link the two.

I haven't tested the results yet, but I expect it to work for the most part. So I guess until later, this is still very much as "use at your own risk" situation since it could be rather buggy now.

Added a compatibility layer `sys_imports.py` which handles imports across different runtimes, and modified classes to use these imports.
Refactored pycopy-based code to match source repo style; requires adding unit tests and examples.
@brainelectronics
Copy link
Owner

Thank you @GimmickNG for your PR 🎊

I need a deeper review and testing the next days but the first impression is very good 👍🏻

  • Check CI run results
  • missing semver valid Changelog entry

@brainelectronics
Copy link
Owner

Please mention #5 and #11 in the Changelog entry

Copy link
Owner

@brainelectronics brainelectronics left a comment

Choose a reason for hiding this comment

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

I've now had the time to review your changes, only minor things to be resolved

umodbus/async/serial.py Outdated Show resolved Hide resolved
umodbus/async/serial.py Outdated Show resolved Hide resolved
umodbus/async/tcp.py Outdated Show resolved Hide resolved
umodbus/modbus.py Outdated Show resolved Hide resolved
umodbus/modbus.py Outdated Show resolved Hide resolved
umodbus/modbus.py Show resolved Hide resolved
umodbus/sys_imports.py Outdated Show resolved Hide resolved
@GimmickNG
Copy link
Author

GimmickNG commented Feb 18, 2023

Thanks for the review. I'll make the changes required (and I also spotted some errors when I tried using the library, whose fixes I'll add here) in a week or so, when I should have more time available.

@GimmickNG
Copy link
Author

GimmickNG commented Feb 25, 2023

I'm not sure why Github doesn't recognize that the async folder was renamed to asynchronous in my latest revision. Oh well. Here are the changes to save time on reviewing:

  • Fixed flake8 errors in both synchronous and asynchronous versions (line too long, incorrect indent level, etc.)
  • Fixed async serial version to be more like async TCP. Requires testing as I do not have a board with UART
  • Breaking change: Renamed Serial class to RTUServer to match TCPServer terminology (since functions are similar)
  • Renamed async folder to asynchronous as importing the package resulted in a syntax error (due to it being a reserved name)
  • Fixed errors with async TCP, encountered during testing on Pycopy/Micropython for Ubuntu. Should work for the most part.
  • Added changelog entries (but since this was a WIP, only mentioned "async support" for all the above changes under the "Added" category)

@beyonlo
Copy link

beyonlo commented Feb 28, 2023

Hello. I'm happy that we are closer to have the async feature ready on the ModBus lib 🥳

I would like to say that I will happy to help on the tests with the serial version (RTU) that require a board with UART. I have one board with RS485 already working! As soon this PR will be merged I can to help do the tests 😃

Copy link
Owner

@brainelectronics brainelectronics left a comment

Choose a reason for hiding this comment

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

you're almost there, very good implementation, love to see this live soon

umodbus/asynchronous/common.py Outdated Show resolved Hide resolved
umodbus/asynchronous/common.py Outdated Show resolved Hide resolved
umodbus/__init__.py Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
changelog.md Show resolved Hide resolved
umodbus/sys_imports.py Outdated Show resolved Hide resolved
umodbus/modbus.py Outdated Show resolved Hide resolved
umodbus/serial.py Outdated Show resolved Hide resolved
umodbus/serial.py Outdated Show resolved Hide resolved
umodbus/serial.py Show resolved Hide resolved
GimmickNG and others added 4 commits March 1, 2023 11:01
Changes `Modbus.process` to use `None` by default to avoid breaking changes

Co-authored-by: Jones <[email protected]>
Revert `RTUServer` to `Serial`, add missing docstrings, shebangs and other metadata, and use named parameters in function calls.

In `asynchronous` TCP and RTU implementations: moved `request` parameter for `send_exception_response` and `send_response` to end of function, and changed them to `Optional` for compatibility.
Copy link
Owner

@brainelectronics brainelectronics left a comment

Choose a reason for hiding this comment

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

LGTM

@brainelectronics
Copy link
Owner

@beyonlo could you test the first release candidate of this lib with Async support?

https://test.pypi.org/project/micropython-modbus/2.4.0rc39.dev56/

@beyonlo
Copy link

beyonlo commented Mar 6, 2023

Hello @brainelectronics

@beyonlo could you test the first release candidate of this lib with Async support?

Yes, of course I can do that.

Are there some example to run the Slave TCP and Slave RTU using the uasyncio, or a simple doc how to use/start it?

https://test.pypi.org/project/micropython-modbus/2.4.0rc39.dev56/

I downloaded this version, but it seems not to have the uasyncio support.

pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56$ ls -la
total 32
drwxrwxr-x 4 pi pi  4096 mar  6 10:38 .
drwxr-xr-x 5 pi pi 20480 mar  6 10:38 ..
drwxrwxr-x 2 pi pi  4096 mar  6 10:38 micropython_modbus.egg-info
drwxrwxr-x 2 pi pi  4096 mar  6 10:46 umodbus
pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56$ cd umodbus/
pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56/umodbus$ ls -la
total 136
drwxrwxr-x 2 pi pi  4096 mar  6 10:46 .
drwxrwxr-x 4 pi pi  4096 mar  6 10:38 ..
-rw-r--r-- 1 pi pi 14741 mar  6 05:04 common.py
-rw-r--r-- 1 pi pi  6785 mar  6 05:04 const.py
-rw-r--r-- 1 pi pi 14778 mar  6 05:04 functions.py
-rw-r--r-- 1 pi pi   108 mar  6 05:04 __init__.py
-rw-r--r-- 1 pi pi 36148 mar  6 05:04 modbus.py
-rw-r--r-- 1 pi pi 17275 mar  6 05:04 serial.py
-rw-r--r-- 1 pi pi  1304 mar  6 05:04 sys_imports.py
-rw-r--r-- 1 pi pi 15704 mar  6 05:04 tcp.py
-rw-r--r-- 1 pi pi  2121 mar  6 05:04 typing.py
-rw-r--r-- 1 pi pi   140 mar  6 05:07 version.py
pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56/umodbus$ cat version.py 
#!/usr/bin/env python3
# -*- coding: UTF-8 -*-

__version_info__ = ("2", "4", "0")
__version__ = '.'.join(__version_info__) + '-rc39.dev56'
pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56/umodbus$ grep -i uasyncio *
pi@pi:~/Downloads/micropython-modbus-2.4.0rc39.dev56/umodbus$

Thank you!

@GimmickNG
Copy link
Author

GimmickNG commented Mar 6, 2023

@beyonlo I am currently writing a few examples for the async clients and servers. However, I noticed that when writing the client and server examples for async RTU, some features in TCP were not present in RTU and had to be implemented manually (such as serve_forever()). Also, starting an async RTU client was made confusing since it required creating an AsyncRTUServer instance, which makes no sense to the end user.

@brainelectronics I am making changes to make the async version more consistent with the sync one for the RTU implementation - namely, changing the regular CommonRTUFunctions to RTUServer and using this to create an AsyncRTU (client) and a proper AsyncRTUServer class, which will be used by AsyncModbusRTU to start an RTU server. What are your thoughts on this, with respect to e.g. backwards compatibility?

@brainelectronics
Copy link
Owner

@beyonlo best finding so far 😂 I've fixed the packaging code, let's try again with 2.4.0rc40.dev56

@GimmickNG could you please update the setup.py as follows

    packages=['umodbus', 'umodbus/asynchronous'],

Regarding backwards compability: As long as the following imports and the top level function names do not change in the sync part of the lib, I'm open to many changes as long as they are related to this PR of course

from umodbus.serial import ModbusRTU
from umodbus.serial import Serial as ModbusRTUMaster
from umodbus.tcp import ModbusTCP
from umodbus.tcp import TCP as ModbusTCPMaster

@beyonlo
Copy link

beyonlo commented Mar 10, 2023

@GimmickNG Thanks for your effort to support uasyncio on this lib. I will wait your changes on the uasyncio version, as well the examples for the uasyncio clients and servers to start the tests!

Will be all features of this lib available/works on the uasyncio version too, including the callbacks functions when Master set/get the address on the Slaves?

Adds examples for async TCP and RTU clients and servers
Refactored the sync and async RTU classes to match the TCP counterparts. Instead of `Serial` being both a modbus RTU client *and* server, the server functionality has been split off into the `RTUServer` class, and `Serial` is now only the RTU client. `ModbusRTU` remains unaffected as its `_itf` is changed from `Serial` to `RTUServer`, but the user-facing API remains the same as they handle the same tasks as before.
Adds async package to the wheel via setup.py
@GimmickNG
Copy link
Author

GimmickNG commented Mar 11, 2023

@beyonlo Since the async version is based off/depends on the sync version, the two should be fairly similar (with the exception of asyncio.run() needed to start the asyncio loop), so yes the callbacks should be supported --- let me know if they aren't.

Also, check async_examples.py for the async TCP/RTU examples. I haven't verified these though so there may be some errors, but I don't expect any initially.

@brainelectronics I split off the server/slave functionality of Serial into a separate RTUServer class since it looked overloaded in comparison to TCP/TCPServer. I tried to keep the user-facing interface as close to the original as possible, so the ModbusRTU class remains the same --- it now uses RTUServer instead of Serial, and Serial is now an RTU client only.

@GimmickNG
Copy link
Author

@modbusrtus Sure, no worries! If you want to scale it across several slave/servers, then you could automate the callbacks by binding the function parameters in an inner function, like so:

async def update_register_definitions(register_definitions, *servers):

    def sync_server_hregs(other_servers):
        def inner_set_hreg_cb(reg_type, address, val):
            for server in other_servers:
                server.set_hreg(address=address, value=val)
        return inner_set_hreg_cb

    for server in servers:
        other_servers = [s for s in servers if s != server]
        for register in register_definitions['HREGS']:
            register_definitions['HREGS'][register]['on_set_cb'] = sync_server_hregs(other_servers)
        server.setup_registers(registers=register_definitions)

Untested, but I believe it should work - it basically generates a callback each time sync_server_hregs is called with the list of other_servers, which is used by the callback to update the other servers' registers. This is then updated across each server in the list, so you don't need to manually define a callback per server. You can repeat the same process for the COILS as needed.

@GimmickNG
Copy link
Author

@beyonlo I think Problem 1 is more related to the one that @modbusrtus had with the shared registers. The callback should be changed to call set_ireg for all the clients, but it doesn't know which clients there are yet. I could modify the examples to include that, but I am not sure if that's OK to do or not right now. For Problem 2, it looks like the old value of the list is being used (since it is a copy), so I have updated the example to retrieve the current value of the register from the server before updating it.

Both of these problems should theoretically be fixed automatically if/when the shared register support is added. That's why I'm not sure whether to make those changes to the examples now, because it might need to be reverted later anyways. For the time being, please use the callbacks that @modbusrtus is using to update the registers for both the TCP and RTUs if you need them to be synchronized.

@beyonlo
Copy link

beyonlo commented Feb 12, 2024

Hello @GimmickNG

Both of these problems should theoretically be fixed automatically if/when the shared register support is added. That's why I'm not sure whether to make those changes to the examples now, because it might need to be reverted later anyways. For the time being, please use the callbacks that @modbusrtus is using to update the registers for both the TCP and RTUs if you need them to be synchronized.

Understood. I agree to create another PR just to add that shared register support after this PR is merged.

Well, so we finish this PR to be merged!

Thank you so much!

@GimmickNG
Copy link
Author

@beyonlo I thought about it and decided to modify the multi_client_example and multi_client_shared_registers examples to share the registers by default using the callbacks that @modbusrtus used. Could you please test those? If those work with the RTU showing the registers changed in the TCP version and vice versa, then I think that this PR is basically complete since there's nothing else that I think needs to be done here currently.

The only downside is that the multi client examples won't have the regular callbacks working, since right now there can only be one callback at a time, and that's being used to synchronize the registers and coils across both slaves/servers. I could modify the sync callbacks to check if there's any existing callbacks and wrap those if needed, but that's going to introduce a lot more complexity for something that'll probably be reverted (hopefully soon) down the line.

@beyonlo
Copy link

beyonlo commented Mar 5, 2024

@GimmickNG Hello, sorry for the long time!

Follow the tests with the new version:

$ mpremote run multi_client_example.py 
Waiting for WiFi connection...
Waiting for WiFi connection...
Waiting for WiFi connection...
Waiting for WiFi connection...
Connected to WiFi.
('192.168.238.177', '255.255.255.0', '192.168.238.111', '192.168.238.111')
MicroPython infos: (sysname='esp32', nodename='esp32', release='1.22.1', version='v1.22.1 on 2024-01-05', machine='Generic ESP32S3 module with ESP32S3')
Used micropthon-modbus version: 2.3.7
Using pins (37, 38) with UART ID 1
Traceback (most recent call last):
  File "<stdin>", line 90, in <module>
TypeError: function missing required positional argument #1

Ps: I did just a fix in the import, because the register_definitions do not exists in the examples.common.tcp_client_common - I already did that in old examples. And I added also the ctrl_pin params where are missing - I already did that in old examples too. Follow the diff from your multi_client_example.py and what I changed:

$ diff multi_client_example.py  /home/pi/Downloads/modbus/04.03.2024/pycopy-modbus-mpmodbus-compatibility/examples/multi_client_example.py 
29c29
< from examples.common.register_definitions import register_definitions
---
> from examples.common.tcp_client_common import register_definitions
40d39
<                           ctrl_pin,
48d46
<                        ctrl_pin=ctrl_pin,
96c94
<                                       ctrl_pin=15,            # optional, control DE/RE
---
>                                       # ctrl_pin=12,          # optional, control DE/RE

Thank you!

@GimmickNG
Copy link
Author

@beyonlo Thanks for testing, and sorry for the delay! I've fixed the error in initializing the RTU server for the three scripts, and should hopefully work now. I don't think you need to add the ctrl_pin part in the start_rtu_server function, you should be able to comment out the # ctrl_pin=12 part (change it to 15 in your case) and it should work since extra args like those will be passed by default to the RTU server object when creating it.

@beyonlo
Copy link

beyonlo commented Mar 29, 2024

Hello @GimmickNG

@beyonlo Thanks for testing, and sorry for the delay! I've fixed the error in initializing the RTU server for the three scripts, and should hopefully work now.

Unfortunatelly that +1 (new_val = val[0] + 1 on the register_definitions.py) does not works[*] on the new multi_client_example.py and multi_client_modify_shared_registers.py examples. But it works on the async_rtu_client_example.py example.

I don't think you need to add the ctrl_pin part in the start_rtu_server function, you should be able to comment out the # ctrl_pin=12 part (change it to 15 in your case) and it should work since extra args like those will be passed by default to the RTU server object when creating it.

You are right about that. I just change 12 to 15 on the ctrl_pin param.

Thank you!

EDIT: Just to be clear with "does not works[*]": They does not show errors, just the Masters does not receive counter +1. The Masters receives always the same number.

@GimmickNG
Copy link
Author

Hi @beyonlo, does the same issue occur with the TCP server example? Also, do you see any messages being called when the get register callback is called, or is the console empty (in which case the getter is probably not being called at all)?

@beyonlo
Copy link

beyonlo commented Apr 24, 2024

Hello @GimmickNG

On the TCP works fine.

On the console shows the called, but reply always the same number, not the number + 1.

* revert changes to examples

* remove multi_client_sync.py
@GimmickNG
Copy link
Author

@beyonlo Sorry for the huge delay. I'm not sure why the serial version isn't working, so rather than hold this pull request up any more I've decided to just revert it to the last known working version. Could you please test this and see if the revert has worked successfully? If so, this could be closed and the multi client sync could be handled later once shared register support is added.

@beyonlo
Copy link

beyonlo commented May 20, 2024

@GimmickNG Unfortunately the team that I was working with in this project changed the priority for another project, and I have no more hardware (485 transceivers to test). This does not mean that we will abandon the project, but I don't know when the lider team will give me time and prototypes again to work on this project.

So, I was thinking to back to our last strategy/conversation to merge this PR and create another PR to fix other problems. This conversation happened in on Feb 12:

image

What do you think?

Thank you!

@GimmickNG
Copy link
Author

@beyonlo Sounds good to me. I restored the files from commit 7ea5124 which was the version just before I made the changes to the multi client examples. So I think it should work since there were only formatting changes made between that version and the one you confirmed was working.

In which case, @brainelectronics could this PR be merged, if there aren't any other comments?

Copy link
Owner

@brainelectronics brainelectronics left a comment

Choose a reason for hiding this comment

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

Let's see what CI will report

@@ -429,7 +430,7 @@ def float_to_bin(num: float) -> bin:
:rtype: bin
"""
# no "zfill" available in MicroPython
# return bin(struct.unpack('!I', struct.pack('!f', num))[0])[2:].zfill(32)
# return bin(struct.struct.unpack('!I', struct.struct.pack('!f', num))[0])[2:].zfill(32)
Copy link
Owner

@brainelectronics brainelectronics Jun 25, 2024

Choose a reason for hiding this comment

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

I assume there is one struct to much, although it's just a comment

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I missed that part when I was adding the struct module back to the imported methoids pack and unpack.

@brainelectronics
Copy link
Owner

I'll try to finally review and test on my side by the end of this week, Jan 2023, ... this change has to be great :D

Copy link
Owner

@brainelectronics brainelectronics left a comment

Choose a reason for hiding this comment

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

So manually running the CI reported no error 🥳
@GimmickNG could you do me one last favour? Please update the changelog.md and package.json a very last time. The version shall become 3.0.0 as some classes got renamed

@@ -81,6 +80,7 @@ def send_response(self,
:param signed: Indicates if signed values are used
:type signed: bool
"""
print("sending sync response...", type(self).__name__, "->", type(self._itf).__name__)
Copy link
Owner

Choose a reason for hiding this comment

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

this is either a logging output or can be removed

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this was mainly for debugging. Removed, thanks!

@brainelectronics
Copy link
Owner

@beyonlo could you test the final release candidate of this lib with Async support?

https://test.pypi.org/project/micropython-modbus/2.4.0rc83.dev56/

@beyonlo
Copy link

beyonlo commented Jul 1, 2024

@GimmickNG Unfortunately the team that I was working with in this project changed the priority for another project, and I have no more hardware (485 transceivers to test). This does not mean that we will abandon the project, but I don't know when the lider team will give me time and prototypes again to work on this project.

Hello @GimmickNG

Unfortunately the team that I was working with in this project changed the priority for another project, and I have no more hardware (485 transceivers to test), kits, and so on. This does not mean that we will abandon the project, but I don't know when the lider team will give me time and prototypes again to work on this project. Until now I did all tests possible in this project, but from now would be great if we found another person to help us on tests. I'm sorry!

@GimmickNG
Copy link
Author

@brainelectronics unfortunately it is not currently possible for @beyonlo to test due to circumstances outside his control. However, I expect the async RTU and TCP should both work as it was restored to the last known working state a while ago.

@beyonlo
Copy link

beyonlo commented Jul 1, 2024

@brainelectronics unfortunately it is not currently possible for @beyonlo to test due to circumstances outside his control. However, I expect the async RTU and TCP should both work as it was restored to the last known working state a while ago.

Hello @GimmickNG Sorry, the message above I replied to you instead to @brainelectronics !

@GimmickNG yes, until this point of PR I already test everything - in many details, so, if the merge was OK, I think that will have no problems in my view. Of course, would be great test again, now in the official lib, but the PR was tested much for me.

@beyonlo
Copy link

beyonlo commented Jul 29, 2024

Hello @brainelectronics Would be great if this PR can be merged. We had a hard work (mostly @GimmickNG), so It would be bad for the library if this PR is forgotten.

@patfrench
Copy link

Hello,
I would also be interested in an asynchronous version.

Can you tell me how to test this PR? Have you made any examples? I'd like to participate (at my level)

Thank you for your work.

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.

7 participants