mcknly / breadboard-os

A firmware platform aimed at quick prototyping, built around FreeRTOS and a feature-packed CLI
MIT License
500 stars 19 forks source link

Unify service arrays into a simple struct array #14

Closed nbes4 closed 2 weeks ago

nbes4 commented 2 weeks ago

Hello! Congratulations on such a cool project πŸ˜ƒ, immediately had to play around with it upon reading about it. After reading into the code to start defining my own services, I noticed that the service arrays could be unified into an array containing simple structs to improve a bunch of things (see below).

Instead of maintaining three different arrays, everything goes into an array of simple structs (see services.c), like:

 const ServiceDesc_t service_descriptors[] = {
    {
        .name = xstr(SERVICE_NAME_USB), 
        .service_func = usb_service,
        .startup = true
    },
    // more service descriptor definitions here ...
};
mcknly commented 2 weeks ago

Thanks for the interest! Your proposed changes to the services infrastructure is logical and I like how it can simplify adding new services. I need a little more time to review, in the mean time I have created a new branch called services-refactor, can you modify your PR to target that branch? I will merge in your code so we have a sandbox before pulling into main. Thanks again for using the project!

nbes4 commented 2 weeks ago

Thanks for your reply @mcknly. I changed the base branch of the PR from main to services-refactor. No stress about the review πŸ‘

nbes4 commented 1 week ago

Awesome, let me know if you find any bugs or discuss the changes.

mcknly commented 1 week ago

@nbes4 I've updated the services-refactor branch with some additional documentation - can you pull the latest and have a look? Start with services.h, read through all the comments, and see if it all makes sense from a new user perspective.

Also, I put a file header in services.c, with your github username as author. If you want it to show anything different feel free to change and submit another PR to the branch.

If all is good I will pull it into main. Thanks again for the contribution!

mcknly commented 1 week ago

Here's the direct link if it's easier...

nbes4 commented 1 week ago

@mcknly Looks good! I added some comments to your commit, don't know if GitHub notifies you of them