Skip to content

lib: add adcs library#140

Draft
alexapostolu wants to merge 2 commits intomainfrom
lib-adcs
Draft

lib: add adcs library#140
alexapostolu wants to merge 2 commits intomainfrom
lib-adcs

Conversation

@alexapostolu
Copy link
Member

@alexapostolu alexapostolu commented Nov 4, 2025

The main purpose of this PR is to find a good bound for code style (comments, documentation, directory structure, etc). The skeleton code I have ispractical and likely will be part of the final satellite code, so I think it's a good example for us to play around with the code style. I wanted to balance between readability, upstream Zephyr guidelines, and practicality.

Any feedback is welcome (too verbose, comments not well formatted, function return values don't make sense, etc..)! And I can add whatever feedback we agree on to our contribution guidelines for future reference

(The other purpose of this PR is to provide one single branch for the ADCS library development and testing, but this should be merged after v0.0.0)

Add ADCS library.

Signed-off-by: Alex Apostolu <apostolu240@gmail.com>
Add ADCS dev board tests. This will only build in CI. To test on
hardware, connect the Nucleo G431rb or the OBC to the ADCS module
via UART, power on and run. Instructions on Notion.

Signed-off-by: Alex Apostolu <apostolu240@gmail.com>
@alexapostolu alexapostolu added the DNM DO NOT MERGE (ONLY THOSE WHO PUT THIS LABEL CAN REMOVE IT) label Nov 4, 2025
Copy link
Member

@mshinjo mshinjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be under driver/ instead of lib/?

@mshinjo
Copy link
Member

mshinjo commented Nov 4, 2025

The main purpose of this PR is to find a good bound for code style (comments, documentation, directory structure, etc). The skeleton code I have ispractical and likely will be part of the final satellite code, so I think it's a good example for us to play around with the code style. I wanted to balance between readability, upstream Zephyr guidelines, and practicality.

Any feedback is welcome (too verbose, comments not well formatted, function return values don't make sense, etc..)! And I can add whatever feedback we agree on to our contribution guidelines for future reference

(The other purpose of this PR is to provide one single branch for the ADCS library development and testing, but this should be merged after v0.0.0)

Sounds good

@alexapostolu
Copy link
Member Author

alexapostolu commented Nov 4, 2025

Should this be under driver/ instead of lib/?

That's a good question. I don't think so because I see drivers as low level code, but ADCS code will contain high level decision making logic (it's almost like application level logic). It doesn't really have any low level code besides sending bytes via UART. Thoughts?

#include <stdlib.h>

/* Datasheet specifics the ID to be 12 bytes, so we store as 16 bytes. */
typedef uint16_t adcs_id_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗️💣 This is 16 bits not 16 bytes

adcs_id_t adcs_id_expected = 0x54;
adcs_get_id_ret ret;

ret = adcs_get_id(adcs_id_received);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:
adcs_get_id(&adcs_id_received);

Comment on lines +28 to +32
/* The get_id function can either fail or not, so return binary value. */
typedef enum {
ADCS_GET_ID_RET_OK,
ADCS_GET_ID_RET_ERR,
} adcs_get_id_ret;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could generalize this by defining a single reusable return enum for the ADCS module:

typedef enum {
    ADCS_RET_OK,
	ADCS_INVALID_ID,
	... // other success/error codes
} adcs_ret;

This approach avoids having separate per function return types and would allow us to have consistent error handling across all ADCS functions. It would make it easier to extend with additional error codes

It allows a consistent interface between module and obc (ie all ADCS functions return with adcs_ret).

But we could leave it for now or decide to go with the per function return types


adcs_get_id_ret adcs_get_id(adcs_id_t *id)
{
*id = 0x54;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the id of the ADCS module 0x54? Is this documented anywhere? We could use a named constant that explains where 0x54 comes from instead of a magic number, or maybe a comment.

/* The get_id function can either fail or not, so return binary value. */
typedef enum {
ADCS_GET_ID_RET_OK,
ADCS_GET_ID_RET_ERR,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, but for future code style should we avoid abbreviations like RET and ERR whenever possible? In this case it is pretty clear but since code completion on IDEs saves you from typing the whole variable name anyways, writing the full words is not really slower.

Suggested change
ADCS_GET_ID_RET_ERR,
ADCS_GET_ID_RETURN_ERROR,

{
adcs_id_t adcs_id_received;
adcs_id_t adcs_id_expected = 0x54;
adcs_get_id_ret ret;
Copy link
Member

@TheArchons TheArchons Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a minor code style nitpick but why do we define ret after declaring it? We can do it in one line:

Suggested change
adcs_get_id_ret ret;
adcs_get_id_ret ret = adcs_get_id(adcs_id_received);

adcs_get_id_ret ret;

ret = adcs_get_id(adcs_id_received);
zassert_equal(ret, ADCS_GET_ID_RET_ERR,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ADCS_GET_ID_RET_OK.

@TheArchons TheArchons added DNM DO NOT MERGE (ONLY THOSE WHO PUT THIS LABEL CAN REMOVE IT) and removed DNM DO NOT MERGE (ONLY THOSE WHO PUT THIS LABEL CAN REMOVE IT) labels Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DNM DO NOT MERGE (ONLY THOSE WHO PUT THIS LABEL CAN REMOVE IT)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants