Fix mask write register

[floitsch]

According to the spec the "Mask Write Register" function ands the
or-mask with ~and-mask.

See section 6.16 of the specification (v1.1b3):

Result = (Current Contents AND And_Mask) OR (Or_Mask AND (NOT And_Mask))

[floitsch]

I couldn't really find a place to add a test. If you let me know where that should go I can try to add it.

[janiversen]

please add a test in the test directory.

[janiversen]

Change is correct, I will approve as soon as a couple of tests are added.

[floitsch]

Can you point me to the file that tests the behavior of the functions?

[janiversen]

look in the test directory! the file names tells what the file test. You are also welcome to add a new file.

[floitsch]

I did look in the test directory, but could not find one test that tests the behavior of the server;
something like "write" and then check that a "read" finds the correct value.
Without your help I'm assuming that there aren't any tests for this.
I will try to write a simple one for this change.

[janiversen]

I never said there were tests specifically for masks, on the contrary I asked you to add tests.

[janiversen]

You could actually just search for the función call.

[floitsch]

I know where the test for the mask is: test_mask_write_register_request_execute.
However, it uses a mock context that discards the values.

I was assuming that there is a test somewhere where the behavior of the server is tested. Something that makes sure that write_coil, followed by a read_coil does actually read the value of the coil without affecting neighboring coils. Stuff like this.
It seems like that's not the case, so I will try to write something myself.

[dhoomakethu]

We do not have functional tests in place yet to test the behaviour of both server and client. For now unit tests should suffice. Thanks for the PR.

[floitsch]

I have added a test.

[janiversen]

Thanks for modifying a single test, this however does not seem sufficient to test the and/or functionally.

[floitsch]

The test uses the 4 nibbles of the 16-bit values to test the combinations:

• and_mask=0, or_mask=0
• and_mask=F, or_mask=0
• and_mask=0, or_mask=F
• and_mask=F, or_mask=F

That should cover all possible mask possibilities.

[janiversen]

Thanks for your contribution (I just added a comment copying your explanation).

We hope you are motivated to do more patches, an easy one, but also a bit bigger is to

• search for :param
• control the :param against the actual parameters
• in case of differences update :param
  This will enhance the documentation quite a lot, so a much needed feature.

[floitsch]

Thanks for merging.

I'm not really a Python coder, and there is plenty to do on our open-source project, so I doubt I will find time to contribute here.
But a good idea to suggest other contribution opportunities.