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

WIP server: allow discriminating responses upon requests's starting address #74

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

piertoni
Copy link

First working code to address #73

It adds an optional argument to server that allows to discriminate between requests.

Example of server:

@app.route(slave_ids=[1], function_codes=[1, 2, 3, 4], addresses=list(range(0, 10)), starting_address=1)
def read_data_store(slave_id, function_code, address):
    """" Return value of address. """
    print(f"read data FC{function_code} slave id {slave_id} address {address} value {data_store[address]}")
    return 0

@app.route(slave_ids=[1], function_codes=[1, 2, 3, 4], addresses=list(range(0, 10)), starting_address=2)
def read_data_store(slave_id, function_code, address):
    """" Return value of address. """
    print(f"read data FC{function_code} slave id {slave_id} address {address} value {data_store[address]}")
    return 1

Result on client

Request starting_address=1 quantity=4 -> [0, 0, 0, 0]
Request starting_address=2 quantity=4 -> [1, 1, 1, 1]
Request starting_address=1 quantity=5 -> [0, 0, 0, 0, 0]
Request starting_address=2 quantity=5 -> [1, 1, 1, 1, 1]

If you are ok with the proposal I can go on finishing with a automatic test and documentation.

As you did before, please give me a quick advice to where put my hand for implement the tests.

@coveralls
Copy link

coveralls commented Aug 30, 2019

Coverage Status

Coverage increased (+0.08%) to 96.382% when pulling 9b2e9df on piertoni:master into d173ab5 on AdvancedClimateSystems:master.

Copy link
Collaborator

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Hello @piertoni,

Thanks for the effort you put into this PR. Your question in #73 and this PR show that the current signature for request handlers is lacking important request information: the starting address and the quantity of the coils/registers that someone requests.

But there is one drawback of this PR: the proposed solution is backwards incompatible. Therefore I am rejecting this PR in this form. I am happy to approve a PR that it adds support for starting_address and quantity in a backwards compatible way.

@piertoni
Copy link
Author

piertoni commented Sep 3, 2019

PR that it adds support for starting_address and quantity in a backwards compatible way

Where exactly is not compatible?
The tests are all passing (except for python3.3 but this is due to some other Continous Integration problem, did you check that?).
My intent is to be fully compatible and this PR in fact is still WIP (as we miss tests and docs).
Please let me know and I will do my best to add this functionality. Just need some some advice from a project maintainer 😄
Thanks

@piertoni
Copy link
Author

piertoni commented Sep 5, 2019

I made a workaround to allow passing partial kwargs to app.route decorated functions.
This is to allow full compatibility with previous code.
Tell me what do you think about this.

Still to do:

  • Proper formatting
  • Tests
  • Docs

@piertoni
Copy link
Author

Just to know if you can check my WIP and if you agree with the idea so I can go on...
There is also to fix the pipeline for python 3.3

@dequis
Copy link

dequis commented Sep 20, 2019

re: failing pipeline, I think 3.3 and 3.4 support should be dropped as they are EOL https://devguide.python.org/devcycle/#end-of-life-branches

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.

None yet

4 participants