Andy Pugh wrote: > Attached are two patch files for your consideration. >
Thank you! I have not tested these patches, only read them, but here's some feedback. In general: there is some whitespace strangeness (tabs vs spaces), and it's lacking documentation (in the hostmot2.9 man page), but over all it looks great! > MesaTPPWM.patch adds support for the 3-Phase PWM function in Hostmot2 > . It is necessary to use the 7i49 I believe, but also potentially > useful for a number of other motor control applications. PCW has been > hinting for a while that it would be a useful thing to have supported. > Briefly: The config parameter is "num_3p_pwmgens" and it is set such > that a value of 0 on any phase is 50% duty cycle to suit wiring to > sine functions etc. I anticipate it being used with higher-level HAL > components. (I can imagine a Hall-input trapezoidal module, an > encoder-input sinusoidal drive etc. These don't exist yet. > In hm2_tp_pwmgen_t, I think there's no need to make the "hm2_tp_pwmgen_module_global_t *hal" dynamically allocated, or even a separate struct. I'd just put the struct right inside the hm2_tp_pwmgen_t. hm2_tp_pwmgen_parse_md() leaks memory in the error paths. hm2->tp_pwmgen.enable_reg is used both as a TRAM register and as a non-TRAM register, which seems like a bug. In hm2_tp_pwmgen_prepare_tram_write(), you should make a local copy of hm2->tp_pwmgen.instance[i].hal.param.scale and verify it's not 0 before you divide anything by it. Gotta keep an eye on those sneaky users, always trying to get us to divide by zero... Looks like you leak some memory in hm2_tp_pwmgen_cleanup() too. And one final nitpick: the module and code are variously referred to as "tppwm", "3ppwm", "3Ppwmgen", and "ThreePhasePWM". I think it'd be easier to follow if you picked one and stuck with it. Not that big a deal, but it would make it easier to read if it were consistent. > MesaLED just adds pins to turn on and off the LEDs on the Mesa cards. > The hm2_led_parse_md() function requires there to be exactly one LED instance. It might be better to make the LEDs optional, so it accepts 0 or 1 instances. The error handling code in hm2_led_parse_md() is wrong, it can leak the hm2->led struct that it allocs. There should be an hm2_led_cleanup() function where you free the memory you allocate in hm2_led_parse_md(). This function should get called from hm2_cleanup(). Thanks for your work on this! And great job on you first git/patch submission :-) -- Sebastian Kuzminsky ------------------------------------------------------------------------------ _______________________________________________ Emc-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/emc-developers
