WIP: Sim add actions to setvalues

[krogozinski]

WIP: Simulator add actions to setvalues

Aims to address #2158:

• Adds actions to setValues() in ModbusSimulatorContext.
• Introduces new arguments of "func_code" and "access_type" to custom actions.
• Existing custom actions will work unchanged. This is achieved by:
  • inspecting the custom action function signature
  • determining if the additional parameters of "func_code" and "access_type" are present, and if so
  • adding them to the action_kwargs (to be updated to action_parameters as per latest in dev branch).

Please note:

• PR is draft
• Still needs to be rebased on dev and squashed
• Opportunities for refactoring exist

Feedback would be appreciated.

[janiversen]

you have merge conflict, please solve.

[janiversen]

A small number of comments (not a formal review):

• I would have expected your changes, would be reflected in the json file as well ?
• you need to provide an example on how to use it.
• "inspect" is a library we will not accept in production code

[janiversen]

Your changes as such looks ok, but it is hard to see the details without a usage example.

[janiversen]

the example needs to be documented in doc/source/library/simulator as a new rst file, and amended to one existing.

[janiversen]

Forgot to write:

I do not understand the need for "access_type", it seems unneeded.

[krogozinski]

you have merge conflict, please solve.

Yes, will resolve as part of rebase and squash.

A small number of comments (not a formal review):

* I would have expected your changes, would be reflected in the json file as well ?

* you need to provide an example on how to use it.

* "inspect" is a library we will not accept in production code

• Yes, will add changes to JSON file.
• Yes, will provide example.
• Understood. Do you prefer to maintain backwards compatibility for user code? If so, I need to find another solution for this.

Your changes as such looks ok, but it is hard to see the details without a usage example.

Yes, will provide an example.

the example needs to be documented in doc/source/library/simulator as a new rst file, and amended to one existing.

Yes, will do this.

Forgot to write:

I do not understand the need for "access_type", it seems unneeded.

Certain function codes such as WriteSingleRegisterRequest (0x06) call both setValues() and getValues(). Providing "access_type" is a way to allow a user action method to choose how to respond to each of these separately.

For example, an action method may only want to be invoked once for a WriteSingleRegisterRequest. In this case, the action method can include a condition if access_type == "set" to ensure it is only executed in the setValues() call inside WriteSingleRegisterRequest.execute().

def custom_action3(_registers, _inx, cell, func_code, access_type):
    """Test action which includes function code and access type as parameters."""
    if func_code == 6 and access_type == "set":
        cell.value = 0x5555

I'm open to any ideas on how to improve this interface or address this issue.

[janiversen]

Yes we need backward compatibility whenever possible!

just make the parameters optional, the existing action should use kwargs if I remember right.

[janiversen]

I understand your explanation of access, but it seems too complex and very theoretical. please do not forget this is a simulator not production.

Only a very special user would ever use it, but all users need to deal with the parameter.

Users wanting something that special can use e.g. inspect to see from where the action is called.

[janiversen]

Thinking about your example, that can be solved without the extra parameter, something like:

def custom_action3(_registers, _inx, cell, func_code):
    """Test action without access type as parameter."""
   global code6_flag
    if func_code == 6:
        if not code6_flag:
          code6_flag = True
       else:
          cell.value = 0x5555
          code6_flag = False

[janiversen]

If your changes are done and merged in time for v3.7.0 (due next week), then adding the function_code is ok.

Otherwise API changes are not allowed until v3.8.0 which are not due for quite some time.

[krogozinski]

Your feedback is very helpful, thank you.

Thinking about your example, that can be solved without the extra parameter, something like:

def custom_action3(_registers, _inx, cell, func_code):
    """Test action without access type as parameter."""
   global code6_flag
    if func_code == 6:
        if not code6_flag:
          code6_flag = True
       else:
          cell.value = 0x5555
          code6_flag = False

This solves the problem nicely. After some thought, another option would be to define a function attribute following the function definition.

def custom_action3(_registers, _inx, cell, func_code):
    """Test action without access type as parameter."""
    if func_code == 6:
        if custom_action3.action_done:
          custom_action3.action_done = False
       else:
          cell.value = 0x5555
          custom_action3.action_done = True

custom_action3.action_done = False

If your changes are done and merged in time for v3.7.0 (due next week), then adding the function_code is ok.

Otherwise API changes are not allowed until v3.8.0 which are not due for quite some time.

I will work on all the points above and aim to issue a PR in time for v3.7.0.

[krogozinski]

In order to include func_code as a parameter, all user-defined functions with named parameters will need to add **kwargs as the last parameter.

For example, the simulator-provided action_increment() function definition will need to change from:

def action_increment(cls, registers, inx, cell, minval=None, maxval=None):

to

def action_increment(cls, registers, inx, cell, minval=None, maxval=None, **_kwargs):
    ...

Do you consider this an API change?

[janiversen]

For 3.7.0 API changes are allowed, and again much later for 3.8.0. API changes needs to be documented in API_changes.rst.

[krogozinski]

I am still working on cleaning up tests and adding to docs. I will wait for the release window for v3.8.0 to open before taking this PR out of draft.

[janiversen]

Thanks for the update. Version 3.8.0 will probably not be before December.

But we do merge Pull Requests with API changes, they simply go into "wait_3_8_0"

[github-actions[bot]]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions[bot]]

This PR was closed because it has been stalled for 10 days with no activity.

[krogozinski]

@janiversen would it be acceptable to re-open this PR? Or is it preferred to open a new PR when a PR goes stale and is closed?

[janiversen]

It is acceptable, but opening a new PR might cause fewer discussions as this one had a couple of problems.....your choice.

[janiversen]

Remark for the upcoming v3.8 API changes are allowed as long as they are documented in API_chsnges.rst, that might make the work easier.

[krogozinski]

Thanks for the feedback. I will open a new PR and describe the required changes in API_changes.rst.