Inverting master no longer inverts slave

[Oblarg]

Not sure if this is intended behavior, but inverting a master no longer inverts its slaves. I feel this is worse than the previous functionality, as it is less-intuitive (to me, at least), requires more code on average, and increases the chance of accidentally opposing motor controllers in the same gearbox (previously, a slave would only be inverted if it needed to run in opposition to the master, which gave a very clear check for such a state that is no longer possible).

If this is intended behavior, I would ask that it be changed. At the very least, documentation should clearly indicate that this has changed as it can cause serious and hard-to-debug problems if it is not noticed.

[JCaporuscio]

I've updated the documentation to emphasize the behavior.

https://github.com/CrossTheRoadElec/Phoenix-Documentation/commit/5827e68331ba549be1e074d26922827b0acea88d

[ozrien]

Documentation has been updated.

If the follower were to also honor the master's invert, than what would follower.SetInverted(true) do? It would either [1] flip the output (again) which will be odd for teams to troubleshoot, [2] do nothing only when motor controller is in follower, which will confuse teams. Also this is not an option since follower and master may require unique inverts.

The invert behavior will not change for 2018, however suggestions for 2019 are welcome.

[Oblarg]

The library used to simply flip the output again, which I (along with basically everyone else whom I have asked for their opinion on this change) found to be far more intuitive; under the previous functionality, slaves were only ever inverted if they ran in opposition to the master.

[ozrien]

If we do [1] then it will mean that possibly the invert of a motor controller will change as it transitions from master to follower. So if master.SetInverted(true) and follower.SetInverted(false), then you decide to make the follower PercentOutput for some reason, you would have to know to change follower.SetInverted(true).

Consider a drivetrain with 2 motors on each side.
-Team codes up four masters for basic drive. -They SetInverted all four to be correct.
-Then team switches from 4 masters, to 2 master with 2 followers. If we do [1] then they have to (again) dial in the inverts.

I would think a better strategy is to setInverted() once, and it works for all configurations all the time.

Last year was different because there was (yet another) separate function for inverting (only in follower mode via reverseClosedLoopOutput). Last year's stuff also did not give the ability to choose what direction you want "positive" motion to be, instead it was primarily a function of how you wired the motor. This means you can't software correct forward/reverse limit switches if they are wired opposite of the motor leads. Now you can, but the compromise is you must tell each motor what direction you want positive-motion to represent.

But I could be sold on [1] for 2019 if there's enough up votes.

[ozrien]

I have another idea that addresses my concern, and meets the OP request.

• If calling application calls master.setInverted(), underlying implem can take note of the call.
• If calling application calls follower.follow(master), underlying implem can auto-forward the invert state by calling follower.setInvert() to match master's invert.
• If calling application explicitly calls follower.setInvert(), implem honors this and no longer auto-forwards inverts on that follower.

I think this would be implemented in CCI. Simple callback strategy will solve this.

This is better than having the follower firmware track it because you can switch control modes on the follower and the inverts don't change accidentally.

Also this means same-invert master/follower groups no longer requires you to SetInvert on followers (because followers will auto-forward it's master invert and whenever the master invert changes)

[ozrien]

Issue added to our private tracker.

[prateekma]

In my opinion, this method of doing things will only make things more confusing than they already are.

[ozrien]

@prateekma You didn't explain why. All you have to do is write two lines of code...

masterTal.SetInvert(true)
followerTalOrVic.follow(masterTal);

...this is exactly what the OP wants.

[solomondg]

This is yet another breaking change to the API, a mechanically breaking one this time. In my opinion, the fallout of this change will cause more harm than good.

[prateekma]

Changing the way motors are inverted each year will cause massive confusion and definitely some burnt out gearboxes if people aren't careful. It can also be unclear if the explicit inversion of the slave is relative to the master or not.

[Oblarg]

To be honest, I don't like the proposed solution at all, if I understand what you are proposing.

Imagine you have a gearbox that requires slaves to run in opposition to the master, and you need to invert the whole thing. You must call:

master.setInverted(true); follower.setInverted(false);

If you neglect the second call, it will not work, even though the intuition is clearly that the "default" status of inverted is false and so the second call doesn't seem like it should change anything. What's worse, calling follower.setInverted(true) (somewhat perplexingly) has no immediate effect on behavior, but has invisibly changed the potential future behavior of the motor.

I believe (and think this is not an uncommon opinion, given my discussion with others) that the original implementation (pre-Phoenix) was the clearest of the three - "inverted" on a slave simply meant "inverted with respect to the master." The current implementation is less-clear, but the proposed change is the least-intuitive yet - compound that with the cost of an additional invisibly-breaking change to an API that has already changed in the past year, and I think this is a bad idea.

[ozrien]

@prateekma I think the cases below cover all possibilities as far as compatibility.

Works the same before and after proposed change

masterTal.SetInvert(true/false);
followerTalOrVic.follow(masterTal);
followerTalOrVic.SetInvert(true/false);

masterTal.SetInvert(true/false);
followerTalOrVic.follow(masterTal);
followerTalOrVic.SetInvert(false/true);

masterTal.SetInvert(false);
followerTalOrVic.follow(masterTal);

Follower would be true w/ proposed change. False o/w.

masterTal.SetInvert(true);
followerTalOrVic.follow(masterTal);

So this would be a problem use-case.

@Oblarg I don't think its good to have SetInvert do something different when it is in follower vs the other modes. I think flipping between PercentOutput-and-calling-set, and Follower should be safe. And it wouldn't be if we did solution [1] (see above).

To get an API that is relative to what master is doing (another flip) for your example, seems like it would require yet-another-inverting-function (gross), which also confused many teams prior to 2018 (we have the support emails to prove it). Maybe if that additional function was named something obvious and very follower-specific that might be better?

[A] So maybe something like follwerTalOrVic.SetFollowerInvert.
At least that isn't back-breaking.

[B] Or maybe SetInvert would optionally take a parameter so caller can better communicate the expectation. "Do you want to flip the hbridge?" vs " do you want to flip opposite of master's hbridge" At least that isn't back-breaking.

[C] Or maybe we do [1], but if the user switches from follower to percent, we could yell at them in the DS.
This would be back-breaking.

[ozrien]

[D] Or we do nothing, and just continue document you should call SetInvert on followers. We could also yell in the DS if SetInvert was not called on a follower. That doesn't answer the flip-against-master vs flip-local-hbridge but might help rookies who don't call it at all.

[Oblarg]

Both options A) and B) sound good to me. They wouldn't break the current API, and would allow those of us who find the "relative" inversion more intuitive to do it that way more easily.

[ozrien]

Quick mock-up here... Should be backwards compatible assuming default FollowerInvertStratey is None.

    class TalonSrx
    {
        enum FollowerInvertStrategy
        {
            None, //!< Do not change invert during follower.  Just honor bInvert.
            MatchMaster, //!< Match master's invert.  Do this if follower motor is mechanically the same orientatoin as master.  Honor bInvert when not in follower mode.
            OppositeMaster,//!< Opposite of master's invert.  Do this if follower motor is mechanically opposite orientatoin as master.  Honor bInvert when not in follower mode.
        }
        /**
         *  @param followerStrategy     Optional param to pick the desired invert strategy when 
         *                              using follower mode. If caller expects follower's invert 
         *                              to be determined by the master automatically, this must be used.
         */
        void SetInvert(bool bInvert, FollowerInvertStrategy followerStrategy)
        {
            // apply bInvert like normal when not in follower mode.
            // when in follower mode check followerStrategy, either in firmware or honored it in the API implem - not sure yet.
        }

        void SetInvert(bool bInvert)
        {
            SetInvert(bInvert, FollowerInvertStrategy.Off);
        }
    }

[JCaporuscio]

Closing the tracker since the public API repo is now up-to-date with the released implementation.