Use typing.Protocol for defining SimAction rather than Callable TypeAlias

[km-64]

The protocol has been made to conform better with the requirement that the SimAction must be an async function:

pymodbus/pymodbus/simulator/simdevice.py

Lines 123 to 126 in 2480640

	if self.action and not (
	callable(self.action) and inspect.iscoroutinefunction(self.action)
	):
	raise TypeError("action= must be a async function")

Using Callable[..., Awaitable[...] currently allows something more general to be passed, so using async __call__ is more inline with the requirement that the action is an async function. Alternatively the Coroutine type could have been used in place of Awaitable. This is because Coroutine is assignable to Awaitable, but not the other way around (Callable is a subtype of Awaitable).

The protocol is also a convenient place to put documentation for each parameter and is easier to understand than the long list of parameters in the Callable:

pymodbus/pymodbus/simulator/simdevice.py

Lines 16 to 19 in 2480640

	SimAction: TypeAlias = Callable[
	[int, int, int, int, list[int], list[int] | list[bool] | None],
	Awaitable[ExcCodes | None],
	]

Note: / was intentionally added to the protocol to make it positional only. This means that it behaves exactly the same as the TypeAlias during type checking. The only exception being that it is more specific to reflect the requirement of an async function, not just a callable that returns an Awaitable object.

[km-64]

Using a Protocol also means that docs can be generated by Sphinx using autoclass:

diff --git a/doc/source/simulator/datamodel.rst b/doc/source/simulator/datamodel.rst
index be1ae396..d35af229 100644
--- a/doc/source/simulator/datamodel.rst
+++ b/doc/source/simulator/datamodel.rst
@@ -39,3 +39,7 @@ Datastore definitions
     :undoc-members:
     :show-inheritance:
     :member-order: bysource
+
+.. autoclass:: pymodbus.simulator.SimAction
+    :special-members: __call__
+    :show-inheritance:

Doing it that way doesn't require any additional extensions for Sphinx, but there are also ways that can take advantage of the autoprotocol directive in sphinx-toolbox.more_autodoc (see: https://sphinx-toolbox.readthedocs.io/en/v2.18.3/extensions/more_autodoc/autoprotocol.html).

However, currently autoprotocol is having some issues with Sphinx 9+ which this project uses, so this improvement can be added later. This is due to the rewrite of some Sphinx autodoc internals that autoprotocol doesn't support yet.

It's possible to add the following line to conf.py as stated in sphinx-doc/sphinx#14089.

autodoc_use_legacy_class_based = True

This fixes the problem with autoprotocol, but using sphinx with python 3.14 will include the __annotations_cache__ special member. It's probably fine to just use autoclass since the result looks almost identical.

[km-64]

@janiversen I can update the docs in a separate PR.

[km-64]

It might also be useful to add sphinx-autodoc-typehints so that the parameter types are documented for functions and methods. It's more of a suggestion, because it does somewhat clutter the docs and the type information is already in the signature.

sphinx-autodoc-typehints also only officially supports Python 3.12+, but the documentation dependencies seem to be configured to support versions below 3.12:

pymodbus/pyproject.toml

Lines 52 to 57 in 2480640

	documentation = [
	"recommonmark>=0.7.1",
	"Sphinx>=7.3.7;python_version<'3.12'",
	"Sphinx>=9.1.0;python_version>='3.12'",
	"sphinx-rtd-theme>=3.1.0"
	]

[janiversen]

I think I wrote this before.

Using a class for a simple function, looks a lot more complicated for users.

It is important that the typing and documentation remains very simple....to make it obvious that writing a simple function is enough.

Please remember typing is optional and not enforced, independent what you write as type you can pass nearly anything.

[janiversen]

It was discussed to do a better parameter check of action() but decided not to.

There is work ongoing to allow sync functions as well, something SimDevice could easily control.

[km-64]

There is work ongoing to allow sync functions as well, something SimDevice could easily control.

Yeah that sounds useful. Adding sync support could be done by just making it a union with a sync Callable type.

I can switch back to the type alias if the goal to keep it simple. The same thing could also be done with @typing.overload with the protocol if at some point you want to use typing.Protocol.

To be more consistent with the check inspect.iscoroutinefunction, Coroutine[None, None, ExcCodes | None] could be used instead of Awaitable[ExcCodes | None] for the return type.

[janiversen]

Sorry for laughing a bit, but your "just" hit me.

The type alias is the lesser part of the change needed.

With async it is easy to do a timeout if the function takes too long, that is quite hard with sync....furthermore e.g. a sync function with db calls, might cause problems im other partsof the code, meaning we get to solve bugs not relatedto pymodbus.

[janiversen]

LGTM, thanks.

[km-64]

With async it is easy to do a timeout if the function takes too long, that is quite hard with sync....

You could try making the sync function into a coroutine with asyncio.to_thread. That will run it in the default executor for the current eventloop. (under the hood it calls loop.run_in_executor).
https://github.com/python/cpython/blob/5afbb60e0283caaf34990bbe8437111ebaae04a4/Lib/asyncio/threads.py#L12-L26

The default executor for the running loop is ThreadPoolExecutor but can be changed with loop.set_default_executor. (I think the threadpool defaults seem fine)

Changed in version 3.13: Default value of max_workers is changed to min(32, (os.process_cpu_count() or 1) + 4).

There is some overhead with switching to a thread and wrapping it in async (maybe not ideal for something called frequently like action), and there are also some considerations about the thread-safety of the contents of whatever the use passes an action.