yaq-project / yaqd-dwyer

yaq daemons for Dwyer instruments and controllers
GNU Lesser General Public License v3.0
1 stars 1 forks source link

16b: order dependence of setting ramp_time and destination #54

Open untzag opened 1 year ago

untzag commented 1 year ago

I can't decide if this is expected behavior or not. Unfortunately this path-dependence makes orchestration clients like rxn_control harder to deal with.

The ramp_time is only accounted for at the time that a new destination is set.

# this will ramp to 100 deg C over one minute
def will_ramp():
    furnace.set_ramp_time(1)
    furnace.set_position(100)

# in contrast, this will not ramp
def wont_ramp():
    furnace.set_position(100)
    furnace.set_ramp_time(1)

I'm thinking I might have a window of 30 seconds for "set_recently". If within that window, setting ramp_time will imply that the controller should change to be ramping to that previous destination.

ksunden commented 1 year ago

Hmmm...

I have less of a grasp on what this hardware is capable of/this application, but I can give a proposal that might work:

When either of ramp_time or position is set, that initiates a ramp from the current (measured?) position to the set position which takes ramp_time. If the two are set back to back, then there is little to no difference in order (unless the system is extremely responsive, which seems unlikely)

If you do not have access to the measured value, it gets more complicated, especially as the ramp time gets small. (Which I think is the most relevant edge case)

My initial instinct was to just say "do the linear interpolation from your previous ramp and start a new ramp from there"... but if ramp_time was 0, then this falls pretty flat, as that would mean that the setpoint is already the destination, and there would be no way for it to know where to start the ramp...

A time window (30 seconds seems large to me, but perhaps that is reasonable for the usecase or it could be a config value) does seem like a relatively reasonable approach, I guess, at least from a practical point of view...

From an API design perspective, I'm not enthused about adding timing of calls as a determinant in behavior, as that seems hard to document and opens to wild hard to reproduce bugs.

I could see an argument for including a set_params(dict) method which allows for simultaneous setting of multiple fields... (this is very familiar in the matplotlib codebase...) I don't love it, as it kind of breaks avro (not completely but it is hard, may have to be limited to primitive types) and the semantics of when to use it are not super clear. (It kind of devolves to the bluesky set method, which is a little less organized than I'd like.)

Perhaps there could be an idea of "atomic operations" introduced (akin to a database), where you call:

furnace.start_atomic()
furnace.set_position(100)
furnace.set_ramp_time(1)
furnace.commit_atomic()

The daemon would then have to cache incoming requests, but not apply them until commit, and do so in a single synchronous operation (i.e. no awaits between calls)

I think this would have to be implemented on core.

And have a cancel_atomic message which throws out the cached values without applying them

And we could have yaqc have a context manager for it to clean up the syntax a bit:

with furnace.atomic():
    furnace.set_position(100)
    furnace.set_ramp_time(1)

Anyway, those are long and somewhat stream of consciousness thoughts.

ksunden commented 1 year ago

Looking at the code, you already do start at the current measured position... could it be as simple as adding self.set_position(self.get_destination()) to set_ramp_time?

Let the physics of the system govern what the timing dependence on behavior is, which I think is to say "if it's set shorter than you can make a noticeable change, you will get expected behavior. if it's set only after there is a noticeable change, then I question your expectations."

I think for most real world applications, this would provide a reasonable (at the very least for computer control) window where order would effectively not matter.