Skip to content

Feature/new integration#32

Open
anthlee396 wants to merge 59 commits intomainfrom
feature/new_integration
Open

Feature/new integration#32
anthlee396 wants to merge 59 commits intomainfrom
feature/new_integration

Conversation

@anthlee396
Copy link
Collaborator

@anthlee396 anthlee396 commented Apr 19, 2025

Added bme68x, aprs, and ax25 libs, configured cmakelist files, and added integration without gps and bme code

Copy link
Contributor

Choose a reason for hiding this comment

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

@thisisnotanELF, @anthlee396

Great work getting this started and making it available to the project. I have a few concerns with this library. It will be important to fix them before flashing to a stm. This library heavily relies on malloc allocations, and I don't see any explicit free function to free up these memory sections. My main concern is that this will lead to memory leaks. Alternative to relying on dynamic allocation, I would consider using fixed-size arrays.

Additionally, the printf statements will cause problems. I spoke with Arie and I know this library was originally prototyped on a PC but it won't work for an embedded application.

Choose a reason for hiding this comment

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

Hey Reese you are correct that there is no explicit garbage collection for aprs.c. I will add a free function to free all the frames which are initialized. My thought was that the user could free the aprs frame which they used and the returned nrzi bit stream. The ax25 library under the hood does the garbage collection for the rest. I have tested the library with valgrind to ensure there are no memory leaks with the dynamic allocation. Finally I was able to run the library on a u0 with the print statements. I will provide a sample program shortly!

Choose a reason for hiding this comment

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

I have added a test program. I will continue to expand it and clean up the aprs lib. My todos currently are wrapping my print statements in defines. Providing a binary to packed uint8_t helper function and expanding the aprs frame types. I will begin working on a decode shortly!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would look into refactoring the malloc stuff. Dynamic memory is generally a big no no in embedded. Static or stack allocated memory will be faster, and easier to work with. If you absolutely want to use dynamic memory allocation, I would look into a static byte pool implementation to guarantee that no memory leaks will happen.

Choose a reason for hiding this comment

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

Sounds good I will work on completing my rev 2 by next Friday and pushing. In the meantime we can test with the current implementation since it does generate valid packets.

@reecewayt
Copy link
Contributor

Added bme68x, aprs, and ax25 libs, configured cmakelist files, and added integration without gps and bme code

@anthlee396 Great work on this. I'm happy to review, but I would suggest other members of your group take a look. This is your project, so it's important that other members know about these changes. @riamahaj and @thisisnotanELF can you please help with this review.

Overall looks good, and I left one comment about the aprs.c library. The other change I recommend is removing the targets directory. The main point of refactoring the code base was to have one central location in the CubeMX directory for code generated by the STM32 CubeMX tool. Also, build.ninja shouldn't be checked in here:

image

Great job and thanks for handling this!

@riamahaj riamahaj force-pushed the feature/new_integration branch from 7ce0940 to 2f0cf11 Compare May 9, 2025 21:02
@riamahaj riamahaj force-pushed the feature/new_integration branch from 9444c9d to 5034687 Compare May 25, 2025 01:39
Base automatically changed from removing-multi-target-proj-structure to main May 31, 2025 00:57
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.

6 participants