I am working on a project using an Argon. I have BLE enabled and am communicating with a smartphone over a UART protocol. So that works great.
I also have a motor that I am running with an encoder signal coming back to the Argon, The encoder signal is coming in on D2, which I have set to trigger an interrupt on a falling edge. The ISR is only 5 lines of code (incrementing a counter).
My problem is – it appears that I am missing encoder pulses in my code. When the motor is running, there is only about 10 lines of code in the LOOP, just checking to see if the counter incremented in the encoder ISR reaches a specific value. If I put a serial.print(counter) in the LOOP function, I see the counter increment by one most of the time, but occasionally it skips a count and shows a counter increment of two…
Does the BLE service have blocking functions that would cause my encoder interrupt not to trigger?
I doubt your interrupt actually loses counts. This would also not fit your description that your printed count occasionally skips forward two instead of the expected one count. This wouldn’t be the case if the trigger was actually missed.
It’s more likely that the application thread is being prevented from servicing your Serial.print() code fast enough to inform you about each individual ISR change due to the BLE thread hogging the controler.
However, when loop() gets executed again it informs you about what happened since last time (i.e. more than one triggers have been collected).
This is why interrupts are used for time critical tasks and the application thread should not be considered to run at a constant/stable pace.
Your loop()must allow for this and be resilient against fluctuations in controller load.
OK – so I moved all motor encoding counters / processing into the ISR.
I’m using the Adafruit-MotorShield-V2 library. WIth the motor running – here is the ISR:
if (winDirection == 0 && wactiveState == 1)
{
// increment counter
wencodecnt++;
if (wencodecnt >= openWinStop)
{
Serial.println("Got To Stop in ISR");
wactiveState = 0; // motor not active
// Turn motor off
myMotor->run(RELEASE);
}
}
else
{
//decrement counter
if (winDirection == 1 && wactiveState == 1)
{
wencodecnt--;
if (wencodecnt <= openWinStop)
{
Serial.println("Got To Stop in ISR");
wactiveState = 0; // motor not active
// Turn motor off
myMotor->run(RELEASE);
}
}
}
This works in the FORWARD direction and stops properly when the encoder count equals the OpenWinStop value. But the REVERSE direction does not work – I see the “Got to Stop in ISR message”, but the motor does not stop running.
You need to follow the state of all your variables to find out what’s causing the behaviour.
i.e. from what direction is wencodecnt approaching your openWinStop comparison?
Since we don’t know what your other code does with the variables used inside the ISR and the over all logic of the application, I can just assume your comparison wencodecnt vs. openWinStop is the culprit.
Assuming your wencodecnt is a signed integer, you’d probably want to have a “dead zone” of +/-openWinStop and you need a way to jump the gap when reversing direction otherwise you’ll always run in a case where wactiveState will be set to 0 and prevent any further increment/decrement and hence ignoring the direction change.
Apart from that I’d propose some code changes for readability.
Instead of these two conditions if (winDirection == 0 && wactiveState == 1) and else if (winDirection == 1 && wactiveState == 1) (the curly braces block for else alone is superfluous in any case here) could be (better IMO) written like this
void motorISR() {
if (!wactiveState) return;
if (winDirection) {
wencodecnt--;
if (wencodecnt <= +openWinStop)
wactiveState = 0;
myMotor->run(RELEASE);
}
else {
wencodecnt++;
if (wencodecnt >= -openWinStop)
wactiveState = 0;
myMotor->run(RELEASE);
}
}
or if you want it even shorter
void motorISR() {
if (!wactiveState) return;
wencodecnt += winDirection ? -1 : +1;
// wencodecnt BETWEEN -openWinStop AND +openWinStop?
if (-openWinStop <= wencodecnt && wencodecnt <= +openWinStop)
wactiveState = 0;
myMotor->run(RELEASE);
}
}
BTW, what is forward and what is backward? You increment and decrement the counter but it’s not clear whether incrementing or decrementing indicates forward.
Can you clarify to remove all doubt?
For even better readability a set of enums and consistent naming conventions might be helpful and if you can change the value of winDirection from 0/1 to -1/+1 you could also drop the ternary operator (condition ? trueCase : falseCase) in my latter code and simply write wencodenct += winDirection.
e.g. like this
enum encDIRECTION {
encFORWARD = +1,
encSTOP = 0,
encREVERSE = -1,
};
const int ENC_LIMIT = 10;
volatile encDIRECTION encDirection = encSTOP;
volatile int encCount = 0;
volatile bool isActive = false; // may become superfluous when encSTOP is used instead
...
void motorISR() {
if (!isActive || encDirection == encSTOP) return;
encCount += encDirection;
// encCount BETWEEN -ENC_LIMIT AND +ENC_LIMIT?
if (-ENC_LIMIT <= encCount && encCount <= +ENC_LIMIT)
isActive = false;
encDirection = encSTOP;
myMotor->run(RELEASE);
}
}
Thanks. Let me add some clarity. The application is using a motor connected to a gearbox to drive a mechanical actuator in a physical world “forward” and “backward” direction. Running the motor forward, we want to increment the counter (wencodecnt) from a starting position to a known ending position (openWinStop) by incrementing wencodecnt on each encoder pulse – and the reverse is true in running the motor backwards from the current position (wencodecnt) to a known stop position (openWinStop) by decrementing the counter on each encoder pulse). The wencodecnt vs. openWinStop comparisons operate correctly given the values set in the main loop before the motor is activated. I haven’t worried about readability yet – but thanks for your suggestions…
The issue is this – things work correctly when we run this the first time in the “forward” direction – the variables are correct and the motor stops when wencodecnt == openWinStop. So far so good. When we then set the variables for running the other direction and run in the “reverse” direction, all of the logic works when we get to wencodecnt == openWinStop, wactivestate gets set to zero, but the motor does not stop running.
Another interesting test we ran – if we replace myMotor->run(RELEASE) in the ISR in both the “forward” leg and the “reverse” leg with code to just set a flag (stopflag), then check the flag in the main loop (outside the ISR), the motor operates correctly in both directions and stops. However, we see a large delay before the motor turns off in the “reverse” direction.
if (stopflag == 1)
{
stopflag = 0;
wactiveState = 0; // motor not active
// Turn motor off
myMotor->run(RELEASE);
}
Is there a problem with the motor library when going from myMotor->run(FORWARD); to myMotor->run(BACKWARD); ? Is there some sort of re-initialization we need to do to change directions?
Thanks for the clarification.
One misunderstanding on my part came from the variable name openWinStop as it does (at least to me) suggest this to be a constant value for both directions, but it obviously isn’t
I’d probably have opted for something like targetCount (merely running in my personal rut )
However, some more questions
which library are you using for your myMotor object?
have you had a look at the implementation of the motor driver library?
have you got all your variables used in the ISR declared as volatile?
what happens when you first go reverse and then go forward again?
When I try volatile int wencodecnt = 0; – the compiler throws an error – …/wiring/inc/spark_wiring_cloud.h:83:25: call of overloaded ‘_variable(const char [18], const volatile int&)’ is ambiguous
On 4) above — if we run the “reverse” function first (before the “forward” function), the myMotor->run(RELEASE); does not stop the motor when executed in the ISR. myMotor->run(RELEASE); does stop the motor if moved to the main Loop, but we see a substantial delay (same as before). Interestingly, if we run the “forward” function immediately after, it works fine with the myMotor->run(RELEASE); in the ISR…
OK some progress. Putting the myMotor->setSpeed(0) before myMotor->run(RELEASE) now causes the motor to properly stop in both the forward and backward directions with these commands in the ISR. Not sure why this would be required but thank you for your advice.
Now back to the original problem. The motor is running beyond the physical point at which it should stop. In our real world setup, I mark the starting (zero) position on our device before executing any motor commands. Then I run the motor for say 5000 encoder pulses, which through the gear train translates to a physical distance on our device. I mark this location and then run the motor in reverse back to the zero point (according to the counter). In every case, the motor runs past the original zero point. If I repeat this sequence of steps over and over, the amount of error accumulates for each forward - reverse cycle.
I was attributing this error to the fact that I previously executing the myMotor->run(RELEASE) in the main loop, which as you pointed out is somewhat unpredictable in terms of when it might get executed, accounting for an error. But now I have all counter incrementing / decrementing, boundary checks, and motor off control in the ISR – so when the terminal count is reached, it should immediately stop the motor.
I had a look in the library and can't understand why either
But the full code might provide some clues
Could the overrun be caused by inertia?
Have you checked when the overrun occures with respect to the actual stop command? If it happens after stop is emitted it would again point at the physical behaviour of your setup rather than code.
One common way to counter such effects is to decelerate when approaching the desired position.
Alternatively you could get a statistical value for "backlash" and "overshoot" and account for that to emit stop X pulses before the system has reached the desired end position.
BTW, with this extra info, have you considered using the difference between current position and target position to indicate desired direction and run state - this way you could get rid of winDirection and wactiveState and the potential inconsistency in the multi dimensional relation between these two values and wencodecnt/openWinStop.
It should also make the code simpler (to write and debug ) and would help with the deceleration approach too.
)
