-
Notifications
You must be signed in to change notification settings - Fork 2
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
eurotherm: test increasing replytimeout #7697
Comments
the acceptance criteria, which could be written a bit better (my fault) were to test the changes with an emulator running lots of sensors. The IOC test is still only using a eurotherm set up for one sensor as far as i can tell? Could you please add the following: Tests should be run against a 6 sensor eurotherm ioc + emulator Currently there is a fixed 300ms delay always added in the emulator, this delay should be configurable via the backdoor for use by a "delay test". There should also be the option for a single short delay i.e. if the emulator sees a single_shot_delay > 0 it delays for this before replying to teh command and then sets single_shot_delay back to 0 so the delay is only for one reply The concern raised by increasing the replytimeout was that it would slow down the ioc polling loops for a large (6 sensor) eurotherm and cause undesirable effects. As the readtimeout rather than replytimeout governs terminating reads, in a normal situation there should be no effect on a 6 sensor eurotherm of increasing replytimeout. The only situation it will cause a change in behaviour is if the eurotherm fails to reply at all, this is goverened by replytimeout and it will now take 2000ms rather than 200ms to spot this and so there will be a larger backup of pending reads before it times out. The effect of this could be tested by setting single_shot_delay to 3s during a test and checking that ioc reading recovers. From observed behaviour of the eurotherm hardware, it looks like it has a random single "pause" every 4 hours or so that was leading to one command timing out. On EMU setting replytimeout to 2s was enough to stop this pause causing a timeout as the hardware itself recovered and replied within 2s. |
As a developer I would like to increase the stream device reply timeout as per #5756 (comment) to avoid this hotfix getting lost on ibex upgrades. It was suggested there that this needs to be tested with a larger eurotherm crate, however the change only has an impact if there is comms issues so maybe an emulator is a better way to test the behaviour? The eurotherm sets up several polling loops and the concern was the behaviour of these if there was a timeout - an emulator could be set to give a random long reply to see the behaviour. A
replytimeout = 2000
seems to resolve issues on EMU, I think an increase inlocktimeout
may be needed to cater for larger sensor numbers due to the additional loops, but unclear.locktimeout = 10000
may be prudent though.Acceptance criteria
Changes tested with emulator running lots of sensors
The text was updated successfully, but these errors were encountered: