Enforcing safe zone in RotatorInterface

[olsonse]

@knro

I've just finished some changes to indirotatorinterface.cpp to unify rotator response. This new patch enforces the safe zone, such that the rotator would not be allowed to travel out of the safe zone to get to a target angle. It may be that the rotator would instead take the longer route to the intended target in order to avoid traversing the non-safe region. One of the side effects of the changes I've made is that all rotators would be commanded to go the optimal direction (shortest path when possible). There are a few issues, as outlined below, with these changes that should be resolved before I submit a pull request. I am seeking thoughts and inputs to remedy these issues.

Background

I've been spending a lot of time fixing the WanderRotator interface to make it substantially more robust. In the process, I have also been looking at some of the intended purposes of the RotatorInterface. In particular, 19a0e9d strengthened the description of the RotatorLimitsNP to represent a safe zone, centered around a reference angle. The intent is obviously to ensure that a rotator can keep cables somewhat stable and untwisted. The problem with the patch in 19a0e9d is that it is possible that a rotator would still move to a new location that is indeed in the safe zone but pass out of the safe zone to get there. One can easily demonstrate this with the Rotator Simulator.

This issue may not happen with all rotators and not with all directions of travel. While some rotator drivers appear to just take the commanded target as is, other rotators modify the commanded motion to select the shortest rotation required to obtain the target angle. It is not clear how many of the rotators may auto-select a different rotation angle internal to their firmware. The rotator simulator, for example, implicitly always travels the shortest distance.

Doing these changes has made me realize that the only way to properly enforce the safe zone is to change MoveRotator such that devices are requested to move in a specific direction to their target.

Now the real focus of this issue:

• All drivers would need to eventually be changed to affect a common response.
• The changes I have made so far cause a breakage in compilation for drivers that are not updated.
• Looking through all the rotators supported in indi and indi-3rdparty, it seems challenging to determine whether all rotators do support rotating in both directions. I would assume so, but cannot find definite documentation.
• It is also challenging to determine actual current response for all rotators since some rotators appear to only be commanded with absolute angles and may not have relative motion control. Possibly resolved by better documentation that I cannot find from vendors.

I think it would be advantageous for RotatorInterface to do as much as possible to unify the rotator response--what my current changes attempt to realize. My current patch modifies MoveRotator to accept both the absolute angle and the signed delta angle for a driver to base its commands on. There are backwards-compatibility issues and I am looking for thoughts before I issue a pull request.

I see the following options:

• Go ahead and modify MoveRotator in all known rotators in indi and indi-3rdparty to take a second argument. For some rotators, it will be clear how to correctly respond. For others, we may choose to ignore the second argument and simply keep responses as they now are. It would be the exercise of others who have documentation and know the other devices better to fix these.
• Introduce an overloaded version of MoveRotator with absolute and relative motion arguments. RotatorInterface would first try calling this new version of MoveRotator and then failover to the old version of MoveRotator if it is detected that the driver did not override the new version of MoveRotator. This also leaves current functionality mostly in place while also leaving the current interface in place. This approach may seem nice because of clean backwards compatibility, but it creates a divergence in the type of interface that RotatorInterface presents as compared with all other INDI interfaces. In order to not have immediate impact to all other drivers, it would have to be a non-pure virtual function. Name hiding in c++ would keep this somewhat invisible to all drivers except for where RotatorInterface calls it. Another challenge for this approach will be how RotatorInterface detects if a driver truly did not override the new MoveRotator overload. A hackish way to do this is for the default version of the new MoveRotator overload to return a bogus IPState, such as -1 statically casted to IPState.

I think I prefer the cleaner first option, even while this will break compilation for any rotators that are not changed. I can fix compilation for all drivers in indi and indi-3rdparty with subsequent patches as described in the first option above. Further, this would force any private driver developers to know that there is a new interface that requests a specific signed delta angle in order to reach a target angle.

[olsonse]

@knro pull request #2432 provides an implementation for my proposed first solution described in this issue. I've tested this quite extensively with the rotator simulator and also with the wanderer rotator--it works very well and cleanly keeps the rotator within the user-defined safe zone.

[knro]

Thanks. I have to carefully check this because not all rotators can move using the shortest path. I tested this for example with ZWO CAA and it always takes the long way to get to target. For example, if I am at 20 degrees and command it to 350, it would always take the long path. If you try to use reverse to move CCW 30 degrees, the reported angle becomes different, it will report 50 degrees and not 350.

[olsonse]

Good to know about the ZWO CAA. As I indicated in the commit log message for the pull request, the only drivers that really take advantage of the new capability are:

• integra
• nframe
• rotator simulator (tested extensively)
• wanderer rotator (tested extensively)

I don't have the integra or the nframe, but they both seemed to obviously support relative motion and already attempted to move the shortest path to the target angle. Now they just use the new delta input instead of computing the shortest path themselves. It would be nice to test these though, but I lack the hardware to test with.

I attempted to lookup documentation for various rotators that I don't have but had a hard time finding enough information.

In my pull request comments, I also mentiond that we would have to update the indi-3rdparty rotators to avoid breaking compilation:

• ASI (ZWO CAA etc):
  • I had already pointed out that this would have to ignore the delta input and just keep the same functionality since, as far as I had been able to tell, this would not work for the ZWO. Good to know that you confirm this.
• seletek:
  • I thought that this would be able to use the new delta information, but after looking at the code more closely, I think this may not be possible. Hence, it would also have to only ignore the delta input.

[olsonse]

One quick question, I realize that this set of patches in #2432 may have an issue with the CI. For most of the drivers, they simply ignore the delta input. I assume that I will have to modify my patch to add INDI_UNUSED(delta) for all the drivers that do not use this input, right?

[knro]

Yes but I don't think we should be passing it to begin with at all. This is a substantial change and might introduce regressions to the code base. Instead of adding this, introduce a new function virtual IPState MoveRotatorRelative(double delta) = 0 and only rotators that can support a true delta movement can implement it. Then for such drivers, simply calculate the delta and then call this function i.e.:

IPState RotatorSimulator::MoveRotator(double angle)
{
     auto delta = calculateDelta(angle);
     return MoveRotatorRelative(delta);
}

[olsonse]

How about the following option that could meet your request to avoid regression issues and also keep the point of this patch intact (of ensuring the safety zone is always at least considered):

We provide a default implementation of MoveRotatorRelative that actually just calls MoveRotator, like:

//In .h file:
/** Drivers can override this function to enable relative motion change, such
 * that the driver would be requested to move in a specific direction and
 * distance.  A driver that implements this function can take full advantage of
 * the cable-neutral safety zone supported by the RotatorInterface and defined
 * by the user while also allowing the driver to support shortest rotations when the
 * rotator is used in an unrestricted mode (i.e. when no safety zone is defined by the user).
 * An unrestricted mode would allow for indefinite rotations in either direction.
 * 
 * The default implementation of this function simply calls MoveRotator(target)
 * where `target` is an absolute angle.  This default implementation does not consider
 * rotating in any specific direction and, as such, does not ensure both fastest motion
 * for the rotator while also keeping the rotator inside the user-defined safety zone.
 * 
 * @param target defines the absolute angle that drivers are requested to move to
 * @param delta specifies the signed relative motion that the driver is requested to
 * move by in order to reach the target absolute angle, strictly honoring the
 * user-defined safety zone.
 */
virtual IPState MoveRotatorRelative(double target, double delta);
/* ... */

// In .cpp file:
IPState MoveRotatorRelative(double target, double delta)
{
     INDI_UNUSED(delta);
     return MoveRotator(target);
}

Advantages to this approach:

1. Minimal immediate impact on current drivers. Only drivers modified to override this function to attempt to follow exact movement instructions will be impacted.
2. Avoids creating a pure virtual function that would require a driver to provide at least some implementation and also avoids the complication of how the RotatorInterface would choose to call one or the other of these functions. I had originally written a response much longer to describe the problems with this approach and could provide more thoughts, but the proposal I am now making is just much cleaner.

Now, perhaps there would be a better name suited for this than MoveRotatorRelative, but I do not have a strong opinion about this.

Finally, a note about the somewhat redundant target and delta arguments to this new MoveRotatorRelative function:
The calling function has to already safely consider these and I feel that it is just better to be as explicit to the drivers as possible, even though the target could be figured out from the necessary delta argument. If nothing else, this just gives the drivers the ability to double check consistency. I had considered that this new function could just as easily have arguments like target and direction where direction was only a +1 or -1. This would avoid the redundancy, but nevertheless, I felt that it was just as well to just pass in the delta angle to just make a drivers job even easier.