2014-12-20 20:11 GMT+02:00 Sebastian Kuzminsky <[email protected] <https://mail.google.com/mail/u/0/?view=cm&fs=1&tf=1&[email protected]>>:
> On 12/19/2014 04:02 PM, Andrew wrote: > > 2014-11-20 21:23 GMT+02:00 Sebastian Kuzminsky <[email protected] > <https://mail.google.com/mail/u/0/?view=cm&fs=1&tf=1&[email protected]>>: > >> > >> > >> If you prepare your work as a git branch (or a patch file) against the > >> linuxcnc "master" branch and send it to the list, we'll review it for > you. > >> > >> > > Please review the attached patch. I'd also like to update sim-hexapod > > config if this goes OK. > > Thanks! > > Looks pretty good to me! > > Your patch applies cleanly to master, builds, the > sim/axis/vismach/hexapod-sim config runs, and the new hal pins show up. > I didn't try changing the hal pins, because i dont really know how > they're supposed to work.... This is awesome. > Thanks a lot for reviewing the patch, Sebastian! > Some feedback on the patch: > > * haldata does not need to be dynamically allocated, it would be simpler > to make it statically allocated. The field a of the struct, the pins > themselves, would still be dynamically allocated by hal_pin_new. > * In the definition of struct haldata, you have a number of fields that > are arrays of 6, like this: > > hal_float_t *basex[6]; > > It would be more clear what's going on if they were arrays of > NUM_STRUTS, like this: > > hal_float_t *basex[NUM_STRUTS]; > Well, that's where I was hesitating. The point is that NUM_STRUTS can only be 6, so once I decided to use 6. Now I see your point. > > * The haldata fields base[xyz] and platform[xyz] are all initialized > twice in rtapi_app_main(): once to 0 immediately after > hal_pin_float_newf(), then again to the DEFAULT_* values from the header > file after all the pins are created. Probably the first one should just > be removed. > > > * The genhexkins_init() function isnt really an init function, it might be > clearer to call it genhexkins_read_hal_pins(). > > I agree > * There are some inconsistencies in indentation. Two places that i > noticed are in genhexkins_init() and rtapi_app_main(). > > > * The manpage should be updated. > > > Nice work, this is a good improvement. Thanks for working on it! > Thanks for your advices! I would like to correct those issues and submit a new patch. Along with a new hexapod-sim config. Best regards, Andrew ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ Emc-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/emc-developers
