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

Bugfix: 'on_get_cb': return value was swallowed #83

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

hmaerki
Copy link

@hmaerki hmaerki commented Dec 24, 2023

Bug

  • If on_get_cb is defined, the second call to self._create_response() overwrites the value returned by on_get_cb.
  • In other words: The value set by on_get_cb was swallowed.

Bugfix

  • Simplified code
  • Call self._create_response() only once.

@GimmickNG
Copy link

I think I did something like this earlier as well. The problem is that caching it this way means that on_get_cb also has no chance to update the vals the way it's implemented now -- so the tests where it calls set_ireg in the on_get_cb might fail, since it expects the register values to update after it is called - but it would end up returning the old value instead of the new one.

@hmaerki
Copy link
Author

hmaerki commented Jan 24, 2024

I think I did something like this earlier as well. The problem is that caching it this way means that on_get_cb also has no chance to update the vals the way it's implemented now -- so the tests where it calls set_ireg in the on_get_cb might fail, since it expects the register values to update after it is called - but it would end up returning the old value instead of the new one.

Hi @GimmickNG
Thanks for your feedback. I could not follow your explanation as I do not know the library sufficiently.
I was struggling due to lack of documentation or lack of examples (or I did just missed important parts).

Reading the documentation/examples, I failed to understand how to implement these concepts:

  • Set synchron (set a value in a 'on_set_cb').
  • Set asynchron (No 'on_set_cb'. Read the value from 'micropython-modbus' when it is needed)
  • Get synchron (In 'on_get_cb', get the the value, lets say from a sensor, and update it) (This does not work - see this PR)
  • Get asynchron: (No 'on_set_cb'. Ppoll for the sensor value and update the value stored in 'micropython-modbus'.)

Eventually, the developer has to decide between these two concepts

  • The state(value) should be maintained by 'micropython-modbus': Then use Set/Get asynchron.
  • The state (value) is maintained outside of 'micropython-modbus', for example in the sensor/actor. Then use Set/Get synchron.

It might be helpful to have the documentation and examples structured this way.

If you like, I could spend some hours in creating

  • Proposal for update to the documentation
  • Proposal for 4 examples as described above

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