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

call async datastore from modbus server execute flow #2144

Merged

Conversation

ilkka-ollakka
Copy link
Contributor

@ilkka-ollakka ilkka-ollakka commented Apr 2, 2024

Take into use of async_getValues and async_setValues in modbus-server message handling, allowing datastore (mainly remote datastore) to use async flow.

Originally split from #2127

@ilkka-ollakka
Copy link
Contributor Author

Currently CI fails, as depending PR's are not yet in place. PR itself is currently edits from getValues -> await async_getValues and setValues -> await async_setValues to make it easier to review.

@janiversen
Copy link
Collaborator

A PR is independent, we do not merge/split PRs.

If a PR is depending on other PRs then please mark it as draft, so nobody wastes time.

@ilkka-ollakka ilkka-ollakka marked this pull request as draft April 9, 2024 11:28
@janiversen
Copy link
Collaborator

I have lost the overview...you have 3 open PR`s, is any of them ready to be merged (all comments resolved) ??

Seems you do not allow maintainers to update the PR, so I cannot rebase the 3 PRs, please do a rebase on the one where you want a final review.

@ilkka-ollakka ilkka-ollakka force-pushed the feat/async_datastore_from_execute branch from 75b9e05 to 7ae8ad2 Compare April 9, 2024 19:09
@ilkka-ollakka
Copy link
Contributor Author

I have lost the overview...you have 3 open PR`s, is any of them ready to be merged (all comments resolved) ??

This one is only one that depends on those 2 other PR's, to my understanding all comments have been reacted.

Seems you do not allow maintainers to update the PR, so I cannot rebase the 3 PRs, please do a rebase on the one where you want a final review.

All are now rebased

@ilkka-ollakka ilkka-ollakka force-pushed the feat/async_datastore_from_execute branch 4 times, most recently from e89ae37 to 2701360 Compare April 16, 2024 09:40
@ilkka-ollakka ilkka-ollakka marked this pull request as ready for review April 16, 2024 10:24
@ilkka-ollakka ilkka-ollakka changed the title call async datastore from execute call async datastore from modbus server execute flow Apr 16, 2024
@ilkka-ollakka ilkka-ollakka force-pushed the feat/async_datastore_from_execute branch 3 times, most recently from 1b40ede to 981ba27 Compare April 23, 2024 07:41
@janiversen
Copy link
Collaborator

Is this ready for review ?

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

It might be that remote returns a value in setValues, but we do not expect a value !

Apart from that the code is currently flawed, because not all getValues() calls check for ExceptionResponse, but this is outside this PR.

@@ -227,7 +224,9 @@ async def execute(self, context):
if not context.validate(self.function_code, self.address, count):
return self.doException(merror.IllegalAddress)

result = context.setValues(self.function_code, self.address, self.values)
result = await context.async_setValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

setValues have no return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -328,12 +332,12 @@ async def execute(self, context):
return self.doException(merror.IllegalAddress)
if not context.validate(self.function_code, self.read_address, self.read_count):
return self.doException(merror.IllegalAddress)
result = context.setValues(
result = await context.async_setValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

setValues do not have a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -68,10 +68,12 @@ async def execute(self, context):
if not context.validate(self.function_code, self.address, 1):
return self.doException(merror.IllegalAddress)

result = context.setValues(self.function_code, self.address, [self.value])
result = await context.async_setValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

setValues do not have a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -213,7 +215,9 @@ async def execute(self, context):
if not context.validate(self.function_code, self.address, self.count):
return self.doException(merror.IllegalAddress)

result = context.setValues(self.function_code, self.address, self.values)
result = await context.async_setValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

setValues do not have a return value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if isinstance(values, ExceptionResponse):
return values
values = (values & self.and_mask) | (self.or_mask & ~self.and_mask)
result = context.setValues(self.function_code, self.address, [values])
result = await context.async_setValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

setValues do not have a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

allows datastore to implement async getValues/setValues to be used with modbus server
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Looking forward to review the datastore you are planning.

@janiversen janiversen merged commit bf3b21e into pymodbus-dev:dev Apr 23, 2024
1 check passed
janiversen added a commit that referenced this pull request Jun 27, 2024
* add _legacy_decoder to message rtu (#2119)

* Add generate_ssl() to TLS client as helper. (#2120)

* ASCII framer using message decode() (#2128)

* SOCKET/TLS framer using message decode(). (#2129)

* Fix decode for wrong mdap len.

* Streamline message class. (#2133)

* modbus_server: call execute in a way that those can be either coroutines or normal methods (#2139)

* Clean datastore setValues. (#2145)

* fixed kwargs not being expanded for actions on bit registers, adjusted tests to catch this issue (#2161)

* datastore: add async_setValues/getValues methods (#2165)

Co-authored-by: Ilkka Ollakka <ilkka.ollakka@cloudersolutions.com>

* Request/Response: change execute to be async method (#2142)

* Bump actions CI. (#2166)

* Fix usage of AsyncModbusTcpClient in client docs page (#2169)

* Sphinx: do not turn warnings into errors.

* Add minimal devcontainer. (#2172)

* Transaction id overrun.

* call async datastore from modbus server (#2144)

* Datastore will not return ExceptionResponse. (#2175)

* Describe zero_mode in ModbusSlaveContext.__init__ (#2187)

* Solve pylint error.

* Show error if example is run without support files. (#2189)

* Fix usage file names (#2194)

* Update client.rst (#2199)

* Transaction_id for serial == 0. (#2208)

* Remember to remove serial writer. (#2209)

* Fix writing to serial (rs485) on windows os. (#2191)

Co-authored-by: jan iversen <jancasacondor@gmail.com>

* test convert registers with 1234.... (#2217)

* Solve serial unrequested frame. (#2219)

* Log comm retries. (#2220)

* prepare v3.6.9.

* pylint.

* Remove python 3.8 from CI.

---------

Co-authored-by: Ilkka Ollakka <14361597+ilkka-ollakka@users.noreply.github.com>
Co-authored-by: sumguytho <48988983+sumguytho@users.noreply.github.com>
Co-authored-by: Ilkka Ollakka <ilkka.ollakka@cloudersolutions.com>
Co-authored-by: Yohrog <69543220+Yohrog@users.noreply.github.com>
Co-authored-by: James Cameron <90580716+jcameron-sso@users.noreply.github.com>
Co-authored-by: Qi Li <160646648+qili-eaton@users.noreply.github.com>
Co-authored-by: andrew-harness <75149647+andrew-harness@users.noreply.github.com>
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