On 5 May 2010 03:34, Sebastian Kuzminsky <[email protected]> wrote: > In general: there is some whitespace strangeness (tabs vs spaces),
I guess I need to lmake my editor match the "style guide", I assume the information is there. > and > it's lacking documentation (in the hostmot2.9 man page), Does that go in the .patch too, or is there another submission path? > 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. At that point I think I was blindly following pwmgen, which presumably is the way it is to support PDM too. > hm2_tp_pwmgen_parse_md() leaks memory in the error paths. Can you elaborate? I have never needed to consider memory leaks before. I guess that you are saying that memory is malloc-ed but not freed in the case of an error? > > hm2->tp_pwmgen.enable_reg is used both as a TRAM register and as a > non-TRAM register, which seems like a bug. It's a bit of an odd animal that register. As far as I can make out bit0 is write-only, bit 1 is read-only and bits 2-31 are unused. I think I have it as TRAM read (for the fault bit) and conventional write. On reflection that probably doesn't make much sense. ... (Other stuff to look at) ... > 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. Yes, that was a bit of a dilemma (or trilemma) for me. I wanted to use "3P" everywhere, but that leads to problems with variable naming. I aimed for it to be "tp_pwmgen" in the code and "3P" in the user-facing bits. I would very much welcome suggestions on a consistent and usable namimg scheme for the whole structure. > 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. I took the view that there is always exactly one physical instance of the LEDs, but perhaps there should be the option to not load the module to save overhead. > 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(). OK, I will look into that.Housekeeping never was my strong point. -- atp ------------------------------------------------------------------------------ _______________________________________________ Emc-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/emc-developers
