Avoid PWM value 255 for TLE5206

[blurfl]

This patch only affects TLE5206 boards. If the PWM value is 255 it is changed to 254. This works around the symptom noted in issue #500

[blurfl]

I've tested this against Maslow boards v1.1, v1.2 and a TLE5206 board. All those pass the GC 'Test Motors' test after this patch. Before the patch, the TLE5206 board did not.

[MaslowCommunityGardenRobot]

Congratulations on the pull request @blurfl

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

[BarbourSmith]

It would be nice to understand what is going wrong and how we broke things, but I support this as a quick fix. I think we might encounter other weirdness down the road until we know what is happening so let's keep our eye on the underlying cause if we do

[blurfl]

Edit - testing with static loads on the motors suggest that coasting may be another cause. Adding a short wait for the motors to coast to a stop before changing direction is another possible patch.

The TLE5206 chip is asserting its open-collector Error Flag, which we connect to ENx but don't pay attention to. (_pwmPin in Motor.cpp, note to self - needs to be initialized INPUT_PULLUP). When the movement commanded by Motor::write() is a start from a dead stop or while the motor is coasting and the direction abruptly changes, the TLE5206 can see the motor windings as a dead short and asserts EF. The EF is cleared by toggling one or the other INx inputs. When the speed is less than full throttle, a pwm signal accomplishes this reset. Watching on a scope with pwm=254, after around 20 PWM pulses the motor has reached a speed where it no longer triggers EF. I haven't looked to see why it worked back before PR#489, but a guess is that back then Axis::detach() was called frequently (in detachIfIdle()?) which would have toggled the INx lines if they were HIGH. This patch would address the EF issue any time Motor::write() is called with max speed. I did find that another way to accomplish this is to put a ~'soft start'~ wait for the motor to coast to a stop in Axis::test() ahead of the second direction movement, like

    if (TLE5206) {
        // let the motor coast to a stop
        maslowDelay(100);
    }

I tested that approach with ~no~ a load on the motors and it worked, ~I'd have to change the setup here to test with loads on the motors~. That would more tightly focus this patch on the 'Test Motors' issue. What do you think?

[BarbourSmith]

Mmmm that makes more sense. So with a load on the motor it coasts less and so the issue is less likely to show up. If we put a delay in there does that solve the issue fully? I would feel more comfortable with a delay than a lockout of 255. Do you think this could be an issue while the machine is running, or does it seem like just a quirk with the test?

[davidelang]

On Tue, 21 May 2019, BarbourSmith wrote:

Mmmm that makes more sense. So with a load on the motor it coasts less and so the issue is less likely to show up. If we put a delay in there does that solve the issue fully? I would feel more comfortable with a delay than a lockout of 255. Do you think this could be an issue while the machine is running, or does it seem like just a quirk with the test?

If we are not going to monitor the error pin, then we need to avoid situations where this is critical. locking out 255 entirely is not that bad a thing.

David Lang

[blurfl]

If we are not going to monitor the error pin, then we need to avoid situations where this is critical. locking out 255 entirely is not that bad a thing.

How about I put up a separate PR with the other approach and let the votes make the choice?

[davidelang]

On Tue, 21 May 2019, Scott Smith wrote:

If we are not going to monitor the error pin, then we need to avoid situations where this is critical. locking out 255 entirely is not that bad a thing.

How about I put up a separate PR with the other approach and let the votes make the choice?

If someone is willing to go to the effort of monitoring the pin and reacting properly (including that it happened), that is clearly the best thing to do. But if we don't have someone who is ready to do that, limiting it to 254 is a quick-and-dirty fix.

David Lang

[blurfl]

monitoring the pin and reacting

Unfortunately all the interrupts are committed for the encoders. In lieu of that where would be an appropriate place to trigger the check of the error flags? One possibility is at the end of execSystemRealtime() which runs from the main loop() as well as from maslowDelay();

[davidelang]

On Tue, 21 May 2019, Scott Smith wrote:

monitoring the pin and reacting

Unfortunately all the interrupts are committed for the encoders. In lieu of that where would be an appropriate place to trigger the check of the error flags? One possibility is at the end of execSystemRealtime() which runs from the main loop() as well as from maslowDelay();

this stays high until cleared, so it doesn't need to be an interrupt.

how bad would it be to wait until the next position calculation loop cycle?

David Lang

[blurfl]

this stays high until cleared

The Axis::test() function doesn't use position calculation, but it does call maslowDelay() which calls execSystemRealtime(). That might be a good enough place to check it.

[davidelang]

this doesn't belong in the test subroutine, this is something that should be in place for any time the motor is being driven.

David Lang

[blurfl]

in place for any time the motor is being driven

Use your 👍 vote here for that

or here for the patch that is confined to the 'Test Motors' issue.

[MaslowCommunityGardenRobot]

Time is up and we're ready to merge this pull request. Great work!