Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat new cs align #422

Merged
merged 11 commits into from
Jul 20, 2024
Merged

Feat new cs align #422

merged 11 commits into from
Jul 20, 2024

Conversation

askuric
Copy link
Member

@askuric askuric commented Jul 14, 2024

This PR introduces a much more robust and verbose current sense alignment. And solves several of the issues that were present in the old version. For example #368

The code sets the voltage first to the phase A and then to the phase B, while measuring the respecting phase currents.

This new code is capable:

  • determining which ADC pin belongs to which driver phase
  • switch the ADC channels automatically
  • inverse the gains automatically
  • deal with _NC channels

Additionally it is capable of determining:

  • Currents are too low for alignement (under 100mA)
  • The ADC channels do not belong to phases of the driver (currents not measured)

The new code outputs more debugging messages and is easier to interpret than the old one.

I've tested the code with several setups and it seems to be much more robust than the old one.

The code is an extension of the PR #421.

There is only one concern that I have with this code and that is the amount of text being printed. Maybe its a bit too much for low memory mcus.

@askuric askuric linked an issue Jul 14, 2024 that may be closed by this pull request
@Candas1
Copy link
Collaborator

Candas1 commented Jul 14, 2024

Hi @askuric

I will have a deeper look when I get home.
Just want to bring 2 topics that are connected and can be tackled together with this PR:

@mcells
Copy link
Contributor

mcells commented Jul 14, 2024

Hi!

I had some problems with the align simply not working reliably with a low resistance (drone) motor.
Even though the voltage should have been high enough to result in a noticable current, the align would still fail.

I attribute this to the duty cycle being so low, that the common mode artifacts of the inline sense amps fell into the adc sampling time, thus creating noise.

So my setup was inline hardware with lowside sensing configured in firmware (with my modified adc code).

What this code change does, is it applies 50% duty cycle (or, more precisely 0.5 * driver_limit) and adds to that the align voltage, thus pushing the pwm edges well out of the adc period. It behaves very much like the normal centered pwm operation we use normally.

Since that change, I've had significantly less problems with the alignment.

@askuric
Copy link
Member Author

askuric commented Jul 15, 2024

Hi guys, thanks for the feedback!

@Candas1 very good remarks!

  • Ok so the gradual ramping up to the voltage necessary seems like a very good idea, I'll add it right away.
  • The center aligned voltage vector also seems reasonable. I am just not sure if it would be better to make it as an option or force everyone using it.
    • Since the currents are gonna be measured using the centered pwm it makes perfect sense to do it here too.

@mcells just as a disclaimer, the old code had quite a few holes algorithm-wise, and there are several cases where the logic would just not work. For example for a long time, the when the pins are switched, the gains and the offsets wouldn't be. I am not sure if this is related to your case, but I just waned to point it out. The new code is more robust and much better tested. :D

@mcells I've got a question.
Did you have similar experiences with the calibration of the gains?
In the new code I've forced the centered PWM for the gain offset calibration, did you try to do something similar?

if(driver_type==DriverType::BLDC)
static_cast<BLDCDriver*>(driver)->setPwm(driver->voltage_limit/2, driver->voltage_limit/2, driver->voltage_limit/2);
// calibrate zero offsets
calibrateOffsets();
// set zero voltage to all phases
if(driver_type==DriverType::BLDC)
static_cast<BLDCDriver*>(driver)->setPwm(0,0,0);

@Candas1
Copy link
Collaborator

Candas1 commented Jul 15, 2024

  • The center aligned voltage vector also seems reasonable. I am just not sure if it would be better to make it as an option or force everyone using it.

We have the modulation_centered option, we could reuse it for the current sense align as well ?
But it 1 by default, so everyone will get it unless they are setting modulation_centered = 0.
And this means same parameter is used for alignment and normal operations.

@askuric
Copy link
Member Author

askuric commented Jul 15, 2024

Ok, so I've added both changes:

  • support for centered pwm using the motor.modulation_centered flag.
  • ramping of the voltage setting to the driver phases (I've ramped it for 300ms, relatively short but I still see a big difference with my motors - it can be longer of course)

let me know what you think.

@askuric askuric added this to the 2.3.4_Release milestone Jul 15, 2024
@Candas1
Copy link
Collaborator

Candas1 commented Jul 15, 2024

@askuric I was doing tests on b-g431-esc1 trying all the combinations of current sense pins and _NC

LowsideCurrentSense current_sense = LowsideCurrentSense(0.003, -64.0 / 7.0, A_OP1_OUT, A_OP3_OUT ,_NC );
I get CS: Switch B-(C)NC, that's correct

LowsideCurrentSense current_sense = LowsideCurrentSense(0.003, -64.0 / 7.0, _NC ,A_OP1_OUT, A_OP3_OUT );
I get CS: Switch A-B, The NC is added only in some cases ? (it's a detail)

Otherwise so far so good.

@mcells
Copy link
Contributor

mcells commented Jul 16, 2024

I must say, I really like the new alignment code. It´s now much more polished, both in terms of coding style and verbosity.

  • @askuric. I tried your code with my "problematic" motor and was not able to provoke the behaviour I was seeing before without modifying the code to disable the minimum current check. But after disabling this check, I was able see the difference between centered and not centered PWM again.

  • I think I was not seeing the issues related to swapping you mentioned, but it´s good that this is now fixed as well xD

  • As for your second question, I have thought about adding this, but believed it to be not worth the hassle*, as the potential for errors should come only from the small dc-common-mode error of the sense amps. But it´s nice to have one less possible source of dc errors now!

  • I think it would be good if the alignment would still run if no encoder is linked to the motor here. I moved the if(exit_flag){ if(current_sense){ ... }} outside of the sensor check, which worked for me.

* In the hfi fork I was/am having problems with setPWM not updating the registers sometimes, meaning applied voltages sometimes "stick"... But no issues in the stock library.

@askuric askuric merged commit 996f312 into dev Jul 20, 2024
45 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

driverAlign() ignores gain sign, always "MOT: Success: 3"
3 participants