I think lines were commented out to make the other parts of the code “find” the right functions on some pins? I’m not sure, that might be a question for @Owen_Williams …
In any case, I think since that time at least the PWM code in SimpleFOC has been refactored so the right pins should be found.
You have to call pinmap_function() with the correct pin-name. So you can pass PB_7 to get the “normal” timer, or PB_7_ALT1 to get the alternate function 1 timer, some pins also support _ALT2 and _ALT3, depending on the MCU…
But these are PinNames, while Arduino-IDE uses digital pin numbers like PB7 (no underscore).
So we’d need to change the API to allow you to use pin-names, or change the API to allow you to pass a timer number along with digital pin ID, or something like that.
The problem is that many Arduino users don’t even use PB7, but rather Arduino pin numbers like 1,2,3, A1,A2, etc… the difference between A0, PA0, PA_0 and PA_0_ALT1 and when to use what will be very confusing to new users.
@robca when I setup that variant for B_G431B_ESC1, it was the first board I’d submitted, it looks like this is more of an oversight than anything else. The PB6/7/8 pins can be used with Hall sensors or encoders and I was using it for Halls. The hall sensor used interrupts that worked without pinmap config. I’d have thought that encoder would have worked too but I’m guessing the hardware approach is more sophisticated (using timers).
BTW - I’m guessing you could always not use this variant and instead use the generic version whose PeripheralPins.c are not disabled. If it’s chosing the wrong timer then comment out the line that maps that pin to that timer so that it can’t find it.
Fixing it properly requires quite a few changes, as far as I can tell. A_ENCODER_A should be defined as PB6 and A_ENCODER_B as PB7_ALT1 in variant_B_G431B_ESC.h, but the digitalPinToPinName() converts everything back to PB_6 and PB_7, as far as I can tell and still doesn’t work
So, for now, I’ll simply hardcode everything in STM32HWEncoder.cpp using the code I posted above, which requires fewer changes to fewer files.
Please ping me if I can help testing new code if you decide to make the HW encoder more generalized. Which would also require testing that both A and B pins are on the same timer, in case the user doesn’t realize it.
I finally managed to get the motor spinning at higher velocity, and I confirmed that using the standard encoder (interrupt driven), the measured speed is correct within a very reasonable error. With the hardware encoder, the velocity returned by encoder.getVelocity() is highly variable between 0 and -100 for a velocity of 100 (negative due to the direction). I do call encoder.update() before reading the velocity and angle
Is it just me? (I’m using the code I shared before and confirmed that the angle is correct even for high number of rotations)
How often are you calling the sensor.update()? There’s a sensor.min_elapsed_time parameter too, perhaps it isn’t being used right in the HW encoder. I’ll check into it.
The velocity calculation depends on the time interval (micros()) and isn’t implemented via the timers. So its precision is limited also by the micros() precision, which can be bad if the intervals are too short.
It also depends on the speed of rotation. if there is no update to the position within min_elapsed_time, then the measured velocity is indeed 0…
I call encoder.update() from the main loop. I suspected that calling it too quickly might cause problems, so I called it every 10 micros() or so, but might still be too frequently. I know that the interrupt-based encoder doesn’t really implement the update() function, but that works even if called too frequently.
But I assumed that the motor itself must call the encoder update every time it gets executed, so I thought that calling encoder.update() every 10 usec should work. It might very well be the wrong assumption.
Shouldn’t getVelocity return the previous velocity if called too quickly? I get 0 frequently, aside from other incorrect values.
Thanks for looking into this, no hurry. Right now the interrupt-based one works reliably, and the G431 is more than fast enough not to have problems with it
Yes, it should so I’m in doubt the code is correct… I have to check but I didn’t get to it yet and now I have to go for dinner I’ll try to play with it tonight
[Edit]
you might also compare with motor.shaft_velocity which is smoothed by the LPF filter…
Oh, and if you’re calling the motor loop, it calls sensor.update(), so its better not to call it again in the same loop iteration…
Did you try to use TIM2 for counting “time” between encoder pulses. ST speak of this in their encoder documentation. I will say it is in fact hardware velocity counting. I have not measured the accuracy compared to other ways, so I hope you will include this approach in your research.
The STM32 HW encoder already uses TIM4 in encoder mode, and automatically counts steps in hardware. To know the encoder position, all one has to do is simply to read the TIM4 CNT register (and handle overflow, it’s a 16 bit timer). SimpleFOC also converts the steps into radians.
The velocity code timestamps two angle readings and the time elapsed in micros, then does some math to return the actual velocity. In theory it’s possible to start a timer when the last reading happened and stop it on the next one, but there would be no value over using the system ticks that run at processor clock speed and have better or same resolution of any other timer
The problem is not having a timer, it’s that the math seems to have problems (I haven’t looked into it yet, aside from a quick review)
Yes, and a way to know how fast the motor is spinning is counting the time between encoder pulses with e.g. TIM2. Each time there is a rising edge on one encoder pin, or on all the pulses, that counter (TIM2) resets. There is a “encoder” timer trigger for that purpose. I am not using that since it resets on all pulses and the MT6835 has too many pulses for it to make sense to trigger on all of them.
The value is simply that it happens in hardware, tied to the encoder pulses which should be evenly scattered around the full rotation.
I suppose the precision depends on the crystal used for counting and how evenly the encoder pulses are in fact scattered. You can set TIM2 to count without pre-scaler and it will count using sysClock.
I see what you mean now. Yes, that would work well, and even ST suggests it in the G4 reference manual
Dynamic information can be obtained (speed, acceleration, deceleration)
by measuring the period between two encoder events using a second timer configured in
capture mode. The output of the encoder which indicates the mechanical zero can be used
for this purpose. Depending on the time between two events, the counter can also be read
at regular times. This can be done by latching the counter value into a third input capture
register if available (then the capture signal must be periodic and can be generated by
another timer). when available, it is also possible to read its value through a DMA request.
But I was hoping to keep most of the current STm32HWEncoder code unchanged.
If I decide to change it, definitely what you suggest seems the best way to go, possibly even using the index pulse. Even if the timer reset happens in hardware, and should not matter how frequently it’s called, so might not be worth having to deal with an extra pin.
This approach does not affect the STM32HWEncoder code. It only uses the same pins as trigger. It does affect the sensor class though. Maybe it can reside as an alternative velocity-code, behind a if statement.
Hey, I’ve checked in some updates to the STM32HWEncoder class, you can now use the alternate timer functions by setting the PinName before calling Init().
Although of course it still won’t work if the lines you need in PinMap_TIM are commented out…
The code for it is already on the dev branch. But I haven’t had a chance to test things yet, I’ve been a bit busy and still have to finish my test setup… but I’ll get to it.
To add a timer-based velocity calculation is on the TODO list . You don’t have to change the Sensor code, just the STM32HWEncoder - it has its own getVelocity implementation.
The problem I see with the code above is that the TIM2 CCR1 register is set by the encoder tick, asynchronously in hardware. But the call to getVelocity() happens at an arbitrary time compared to this - and there is the possibility that the value in TIM3 CR1 doesn’t fit to the value?
I also wonder if it is true that every tick is the same distance - I think that’s only true when continuing in the same direction, but not when reversing direction?
Perhaps these things aren’t a problem in practice…
So I’ve checked in bug-fixes to the STM32HWEncoder. It is working for me now.
It was working before at lower resolutions, but I’m now testing with the MT6835, which can do up to 16384 PPR for 65536 CPR - which is 16 bits and also the range of many of the STM32 timers.
It wasn’t working at higher resolutions. It’s working for me now at 8192 PPR and even 16000 PPR is working fine.
But 16384 PPR (the max) is still not working. The current initialisation only works when CPR < 65536. Looking at the code I think this can actually be fixed. I wonder if there is any value in supporting PPRs > 16384? This would require using a 32bit timer.
I also wonder if there is any value in the way it handles the CPR - basically trying to use the timer’s range as much as possible. So in a 16 bit timer, with a CPR of 20000, the code would let the timer run to 60000 before overflowing. It’s seems somehow complicated, and means STM32HWEncoder needs to override update(), getVelocity() and the other methods.
It seems to me if I just set the timer limit equal to the CPR I could get rid of all this custom code and just let the sensor base class handle update() getVelocity() and everything else in the normal way?
We’ve already discussed the possibility of implementing velocity based on another timer. I will look into adding it.
I also wonder if we can track the full rotations using another timer. If we set the encoder timer TOP equal to CPR, then it will generate overflow or underflow events corresponding to the full rotations. I wonder if another timer can track those?
If so, this would be cool because full rotations could be tracked reliably without the need to call update() at high frequency.
However, we’d have to read angle and full rotations from separate registers, so there could be the same concurrency issues as when reading velocity from a separate register. I wonder if ARM has a way to ensure atomicity of these memory reads in the face of timer events?
Please excuse the long rambling, this is half in case someone is interested, and half as notes to my future self
with the current sensor.h definition of float min_elapsed_time = 0.000100; // default is 100 microseconds, or 10kHz, on a G432B_ESC to loop is too fast and velocity is never calculated (due to the dt being too small)
The initialization works better now, but at least for G431B_ESC there are still issues. If I define the encoder pins as PB_6 and PB_7_ALT1, somewhere along the way they get changed. I edited the pin definitions in the config header files, but I might have done something wrong
oooh, thanks for finding it - that will be due to the custom getVelocity again.
I think I will change the code as I described above, to just use the timer and set the timer limit to the CPR - then the timer will overflow once per full rotation, and I’ll just let the Sensor base class handle velocity and full rotations in the normal way. If the code had been that way from the beginning you would not have all these problems.
You have to pass normal pin-ids (PB6, PB7) to the constructor. Afterwards, you can set the pins manually using sensor._pinA = PB_6 and sensor._pinB = PB_7_ALT1. Then you call sensor.init(), and if your PinMap_TIM is correct, everything will work.
I’ve tried it out myself using alternate timers and this works. So if you’re still having troubles with this procedure, I think it now down to the incorrect PinMap.
ooh, clever. Yes, that works, I edited the class to define .pinA and .pinB, did not think of defining it before calling the init() function. Ideally, the constructor should be able to handle the alternate pins, but this way works well with your code unmodified. Thanks!
Just in case someone else reads this in the future, for the G4_31B-ESC board, after editing the pinmap file (PeripheralPinsB_G431B_ESC1.c) to uncomment PB_6 and PB_7_ALT1, the code is
I’ll try to play with it tonight