Skip to content

Conversation

@heeplr
Copy link
Contributor

@heeplr heeplr commented Dec 10, 2023

To deduplicate some repeating code, this PR introduces a formatting hal_export_functf() similar to the hal_*_newf() functions.

Existing occurences of static char buf[HAL_NAME_LEN+1]; ... ; rtapi_snprintf(buf, ...); hal_export_funct(buf, ...) were replaced.
Additionally, some unused/buggy code was removed from boss_plc.c.

@andypugh
Copy link
Collaborator

This seems like a great deal of trouble to have gone to to save a few keystrokes on the rare occasions that someone writes a new HAL component.
I have re-started the CI checks as I think I managed to fix them yesterday.

@heeplr
Copy link
Contributor Author

heeplr commented Dec 11, 2023

This seems like a great deal of trouble to have gone to

Absolutely not. Any decent IDE should make such a PR an easy task. (I would still betatest such a PR before including it in a stable release ;)
If there's anything for me to make the review process easier, please let me know.

to save a few keystrokes

This surely isn't the best example since it's not a critical part of the code (I don't feel familiar enough with linuxcnc for that yet) but I see multiple reasons beyond saving keystrokes why cleanups like this are important for a codebase in general:

  • API consistency - why are there formatting *_new() functions so you don't have to do your own snprintf() but for other stuff you have to do your own snprintf() even if it's just for simple string formatting?

  • avoiding duplicate code - it unnecessary blows up code and makes it harder to read. Saving keystrokes for readers is much more important than for authors. Anything that works and reduces lines of code is a good thing.

  • avoid repeating unnecessary pitfalls that can cause bugs - This is not exactly trivial: Please notice the missing +1 and the missing ENOMEM check here compared to other drivers, causing a potential overflow because driver developers have to take care for that, although the HAL api should do it for them.
    Surely minor but any source of error removed is a good thing. Driver developers shouldn't be able to cause errors in non-driver code.)

on the rare occasions that someone writes a new HAL component.

Adding HAL components probably will be the only thing in linuxcnc that cannot ever be feature complete. As long as people build new hardware, there's new HAL components, right? The project should encourage new components and strive for a feature complete API.

@andypugh
Copy link
Collaborator

Maybe I wasn't clear. I am not proposing to reject this, just expressing surprise that you chose this to work on when LinuxCNC is broken in so many other ways.

@andypugh andypugh merged commit 2c32f7e into LinuxCNC:master Dec 11, 2023
@heeplr
Copy link
Contributor Author

heeplr commented Dec 11, 2023

Maybe I wasn't clear. I am not proposing to reject this, just expressing surprise that you

ah! I'm sorry. I totally didn't get it.

chose this to work on when LinuxCNC is broken in so many other ways.

That's actually an important point imho, hence I'll elaborate briefly:
Until now I just fixed the stuff that was broken for me. Some fixes affecting substantial issues (e.g. #2718 and #2767).

But currently I'm not profiting from fixing other stuff. Despite seeing potential for a lot of fun, working on the linuxcnc codebase on free time isn't exactly fun.
I think, low hanging fruits like formatting the code, more CI (e.g. testing ARM platform stuff) and more tests (so we can poke around in the code without walking on eggshells) should come first and would lower hurdles for larger contributions significantly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants