ngscopeclient / scopehal

Test and measurement hardware abstraction library and protocol decodes. This is the library only. Most users should use scopehal-apps.
BSD 3-Clause "New" or "Revised" License
199 stars 89 forks source link

Proposal for dynamic rate limiting on SCPI communication #903

Open chille opened 6 days ago

chille commented 6 days ago

The problem

When writing the driver for the OWON HDS200 I encountered a problem that had no perfect solution.

Some commands, like :DMM:MEAS?, can be sent without any rate limiting. When requesting data with SCPITransport::SendCommandQueuedWithReply() the call will be blocked until the data is returned. It is then safe to immidiately sent another command.

Other commands, like :DMM:RANGE {V, mV} takes a really long time. As the command is write only, there is no waiting for a reply. If a request is sent directly after, then it will either get dropped, or might possible get the wrong data. I currently block for around 800 ms to avoid this.

This problem have a few possible solutions:

1) Do all the rate limiting with usleep(). This might freeze the GUI for a couple of seconds when doing things that takes longer time.

2) Use the already built in rate limiting. Then everything will be slow. A :DMM:MEAS? will do something like 1.2 readings per second, instead of 15. Other things like the scope might be even worse.

3) Extend the SCPITransport to have some kind of dynamic rate limiting. Something where each command could have a "settle time" that will block the next command until it is safe to run. This is the solution I propose.

How do we implement this?

API changes

All SCPITransform::Send*() functions will have an extra argument added: std::chrono::milliseconds settle_time = chrono::milliseconds(0) This settle_time is optional and will default to 0 ms. This will not need any refactoring in other classes.

Storing the required data

SCPITransport::m_txQueue will be changed from: std::list<std::string> m_txQueue; to std::pair<std::string,std::chrono::milliseconds> m_txQueue;

In this way we can store both the command and the associated "settle time".

Nothing in the scopehal-apps repo uses m_txQueue outside of the SCPITransport class. So this should be easy to change.

Sending data

All code that does some kind of timing in SCPITransport should do it like this instead:

If settle_time > 0
    Use settle_time
Else
    Use m_rateLimitingInterval

I have not yet looked into what exact stepts that will be needed for this. It could possibly be as easy as letting RateLimitingWait() be aware of the settle_time.

Notes

azonenberg commented 6 days ago

This looks like the right way to do this. Go ahead and throw together a prototype implementation and see how it works for your hardware?

The transport object model was designed to be pretty self-contained for exactly this reason, so it should be a fairly straightforward change.

chille commented 6 days ago

Great! I will continue with the development and prepare a pull request when ready!

chille commented 3 days ago

I have successfully implemented and tested the dynamic rate limiting API. I will continue the development of the OWON HDS200 driver to test it a bit more and then open a pull request.

While doing so I have found a few more things we need:

1) Some way to make all calls always be blocking. Basically something that "auto flushes" the command queue immidiately. Otherwise this will cause a lot of extra work when writing your own application for automated testing linking with scopehal. If I dmm.SetMeterMode(Multimeter::DC_VOLTAGE); then I want to be sure that the command is sent immidiately and is blocking until it is safe to do a dmm.GetMeterValue();. I have solved this right now by using the SendCommandImmediate*() functions. But a queue with the option to enable auto flush might be a better idea, as it will be thread safe.

2) We really need some way to be able to add an additional delay to commands. There is a lot of trickery going on in the OWON HDS200 driver right now. I can properly time the massages on my host, but a router in between me and the instrument might merge these two packets together before reaching the instrument. If this happens some things will break. This is of course impossible to avoid. But to have the option to "delay all packets an extra 1200 ms" might mitigate the problem enough. This should probably be in the SCPISocketTransport class. This will also give us the ability to "simulate a slow VPN" as debugging/development feature.