add 'change_rate' randomization option #1229
Conversation
|
Let’s see what CI thinks |
|
Sry just fixed on of the CI error, can you trigger again ? |
|
if you have the newest dev, you can run the checks locally with ./check_ci.sh I am off for the next 8-10 hours. |
|
any idea why the CI / black is failing ? |
|
Look at the details in the log, it shows what needs to be changed.
Or run check_ci.sh it makes the changes and you just need to commit them.
El El mar, 13 dic 2022 a las 8:07, Chris Hung ***@***.***>
escribió:
any idea why the CI / black is failing ?
—
Reply to this email directly, view it on GitHub
<#1229 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPGDL57A665HI3PWXS3WF3WNAOD5ANCNFSM6AAAAAAS4WLVEE>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Sent from My iPad, sorry for any misspellings.
|
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
I will leave it to @dhoomakethu to decide, which help is better.
pymodbus/repl/server/main.py
Outdated
| 0, | ||
| "--change-rate", | ||
| "-c", | ||
| help="Chance of randomizing reads. 0=never, 100=always, 1=1%" |
There was a problem hiding this comment.
| help="Chance of randomizing reads. 0=never, 100=always, 1=1%" | |
| help="Rate in % of registers to change. 0=none, 100=all, 12=12% of registers" |
I believe this is a better help.
There was a problem hiding this comment.
IMHO, --change-rate ,--probability or simply --likely are all OK.
chr1shung
requested review from
dhoomakethu and
janiversen
and removed request for
dhoomakethu and
janiversen
| max_register_value = kwargs.get("max_register_value", 65535) | ||
| self._randomize = randomize | ||
| self._change_rate = change_rate | ||
| if self._randomize > 0: |
There was a problem hiding this comment.
this will potentially confuse a user, please add a logger message so the user knows why -r did not work.
There was a problem hiding this comment.
After the new commit, do you have any suggestion on how to handle the case that user provide both flags ?
There was a problem hiding this comment.
My personal preference is to terminate with an error message, but at the very least the user needs to be aware that the -c is voided.
pymodbus/server/reactive/main.py
Outdated
| self._read_counter[_block_type] += 1 | ||
| elif self._change_rate > 0 and _block_type in {"d", "i"}: | ||
| for offset in range(count): | ||
| if random.randint(1, 100) <= self._change_rate: |
There was a problem hiding this comment.
This will work but not with the effect you described. E.g. -r 50, and 100 registers….randint will be called 100 times but potentially deliver no value below 50.
Please either correct or correct help.
There was a problem hiding this comment.
I push a new commit per @dhoomakethu's suggestion and I think that solved the problem
| :param unit: Unit id for the slave | ||
| :param single: To run as a single slave | ||
| :param randomize: Randomize every <n> reads for DI and IR. | ||
| :param change_rate: Rate in % of registers to change for DI and IR. |
There was a problem hiding this comment.
Your code do not work in % but random, that is every run will change a different number of registers some will change more registers than -r % and others fewer.
There was a problem hiding this comment.
I push a new commit per @dhoomakethu's suggestion and I think that solved the problem
|
The failing test is not due to your code, it is a CI problem. I merge this now, please make a new PR for what you decide to do with overwriting -r |
3 participants
No description provided.