simplefoc / Arduino-FOC

Arduino FOC for BLDC and Stepper motors - Arduino Based Field Oriented Control Algorithm Library
https://docs.simplefoc.com
MIT License
1.97k stars 513 forks source link

[BUG] Potential for memory leaks #255

Open txf- opened 1 year ago

txf- commented 1 year ago

Unless I'm mistaken I've noticed a potential for a memory leak when instantiating current sense parameters for example looking at

stm32g4_mcu.cpp I see:

void* _configureADCLowSide(const void* driver_params, const int pinA, const int pinB, const int pinC){

  Stm32CurrentSenseParams* cs_params= new Stm32CurrentSenseParams {
    .pins={(int)NOT_SET, (int)NOT_SET, (int)NOT_SET},
    .adc_voltage_conv = (_ADC_VOLTAGE_G4) / (_ADC_RESOLUTION_G4)
  };
  _adc_gpio_init(cs_params, pinA,pinB,pinC);
  if(_adc_init(cs_params, (STM32DriverParams*)driver_params) != 0) return SIMPLEFOC_CURRENT_SENSE_INIT_FAILED;
  return cs_params;
}

Stm32CurrentSenseParams are instantiated on the heap and the pointer is assigned to a void* member in the calling class. I don't see any way that if the caller is destructed that this memory will be freed appropriately. If the top level class is only instantiated once and has a lifetime of the entire program then it's not really a issue, but if one would happen to want to use these functions dynamically then this would be bad.

Presumably the most workable solution would be for these drivers to implement a function that frees the memory, to be called in the destructor of the top level class?

something like:

_deinitADCLowSide(void* cs_params)
{
   delete static_cast<Stm32CurrentSenseParams*>(cs_params);
}

and then _deinitADCLowSide() would be called in the LowsideCurrentSense destructor.

Describe the hardware setup For us it is very important to know what is the hardware setup you're using in order to be able to help more directly

IDE you are using Platformio

runger1101001 commented 1 year ago

Hey, thank you for reading the code so carefully, and you are of course 100% correct!

however, you’re also correct in your hypothesis that the intent is that this only happens once, globally.

while of course we can’t see exclude the possibility of other use cases, we do expect that normally current sensing and PWM are configured once, during Initialization, and then never changed. This is also because they are closely linked to the hardware used.

but in the longer term we should probably clean this up, so thank you for reporting it!