You are here

[Contributing][FOC] injected ADC and code clarity

2 posts / 0 new
Last post
Guillaume227
Offline
Last seen: 3 months 2 weeks ago
Joined: 2017-09-11 09:56
Posts: 3
[Contributing][FOC] injected ADC and code clarity

in the FOC code (mcpwm_foc.c), there is a variable called last_inj_adc_isr_duration.

My understanding, looking at the mcpwm_foc_init method is that injected ADC are not used (they are used in BLDC mode). Only regular conversion is used.

Also, that variable is updated in mcpwm_foc_adc_int_handler whereas the corresponding bldc variable is updated in mcpwm_adc_inj_int_handler, so it's all a bit misleading.

To make things more complicated, there is an interface method that blends the two concepts (mc_interface_get_last_inj_adc_isr_duration).

Am I missing a higher level symetry between the 3 modes here, or is indeed that naming detrimental to the clarity of the code?

Are contribution of a "code clarifying" nature welcome (I see on the ethos page that it's good practice to discuss it here before flooding github with push requests)?

Otherwise, my hat is off to that amazing piece of work.

 

 

Roger Wolff
Offline
Last seen: 2 months 3 days ago
VESC FreeVESC Original
Joined: 2017-05-24 12:27
Posts: 202

The code has "evolved" so somehow the inj adc function time was important when BLDC was the only mode and then when FOC came along it had the regular adc interrupt used that same timing measurement variable. 

But nowadays the naming of the variables and functions is not reflecting the truth. 

 

I would be a proponent to clean up such code once and for all. 

 

recording the time that a critical function takes: Starting with the usage: 

DEBUG_RESET_TIMING_TIMER ();

... code to measure ...

DEBUG_RECORD_TIMING (TIMING_MCPWM_INJ_ADC);

Note that these are defines. We can compile the code to make those evaluate to nothing to get slightly better performance. 

 

Now.... storing the results can be done in an array when the identifiers in the "RECORD TIMING" function are declared as an enum starting at zero. And now a header can simply control what is actually recorded: 

#ifdef TIMING_RECORD_SINGLE_ID

int last_timing_result;

#define DEBUG_RECORD_TIMING(id) if(id == TIMING_RECORD_SINGLE_ID)last_timing_result = get_timing_timer_value();

#else

int last_timing_results[TIMING_MAX_ID];

#define DEBUG_RECORD_TIMING(id) last_timing_result[id] = get_timing_timer_value();

#endif

Now, the code will be clear that a timing is being recorded, but we can centrally decide to compile in this code or not at all. Maybe we can optimize the single ID to not do a runtime test. To eliminate the code for not-timed start-of-section that would need to be passed the ID too. 

 

For the record: I think it is better to have code that does not have many #if #else  clauses. You can define things to evaluate to nothing and arrange the complicated if this then that stuff somewhere hidden in a header file.