Closed ddkohler closed 1 year ago
Removing child's get_limits
and in_limits
methods makes bug go away, I propose we remove get_native_limits
, reserve config's limits to native values, and write in the documentation that get_limits
returns in native values.
I am against having limits refer to native values, because it goes against the idea of the has-transformed-position trait. A central idea of the trait is that a daemon's position
can be thought of in some some experimentally convenient scale distinct from the specifics of the motor itself. So reporting limits back in some scale that position does not refer to is confusing.
So here is my proposal:
get_limits
reports the limits of position (i.e. the transformed position)native-limits
, which lets the user input limits in the coordinates of the native motor scale. Users can config limits as either limits or native-limits, and the daemon will look for the intersection of both ranges.Note that this proposal is all about how the client interfaces with the daemon, and has nothing to do with how we keep track of position under the hood. Case and point: has-transformed-position is implemented in a way that keeps track of native position under the hood. This detail of the implementation might be the key to figuring out what changes are needed to make limits work correctly.
I think the use of super()
is the inherent problem with the code. We may have to remove these references and completely override set_position
, in_limits
, get_limits
, and possibly get_position
and get_destination
for both HasTransformedPosition and HasLimits classes. ("Completely" --> not use super()
) Are you ok with that?
The main issue I was discussing was how the client side should not see changes. I'm not super concerned with how the daemon is coded under the hood. Any fix is a good starting point.
I think that in order to have an easy to understand set of classes like this we may have to enforce the rule that both native_limits and limits are specified in the config file when using has-transformed-position
. I think this is more important than attempting to make the set up more user-friendly.
we may have to enforce the rule that both native_limits and limits are specified in the config file when using
has-transformed-position
I think you are saying the same thing I said earlier? I am not sure I understand the part about enforcing limits be specified--both native-limits
and limits
should be optional arguments with defaults of [-inf, inf].
ok I understand now
Some summary thoughts (for future reference):
HasTransformedPosition
wraps messages of parent classes HasLimits
and HasPosition
. Clients send/receive messages in transformed coordinates, and the wrapper transforms them into native coordinates and passes on to the parent class methods. The parent classes work in native coordinates.HasLimits.set_position
, which sets position in native coordinates, called self.get_limits
, which now points to HasTransformedPosition.get_limits
, which gives limits in transformed coordinates. This jumbling is at the heart of the issue here.HasLimits.set_position
, now calls self._get_limits
(note the underscore), which need not be wrapped by HasTransformedPosition
.
Currently, limits mixes up native and transformed limits when checking what positions are valid.
e.g. position = 2 * native limits = [1, 2] (this should apply to transformed position)
calling
set_native_position(0.5)
will actually set the native position to 1, and the transformed position to 1.5In addition to fixing this bug (native should only check limits in native coordinates), I propose to add new config params for the trait that allow setting limits in native position. In software, we will always restrict movement to the intersection of valid native positions and transformed positions.