Skip to content

State Estimations & Telemetry Updates - #47

Open
ETSells wants to merge 10 commits into
mainfrom
integration/state-estims-telemetry
Open

State Estimations & Telemetry Updates#47
ETSells wants to merge 10 commits into
mainfrom
integration/state-estims-telemetry

Conversation

@ETSells

@ETSells ETSells commented Jul 6, 2026

Copy link
Copy Markdown
Member

Description

Monster PR batching issues from Epic SunDevilRocketry/Flight-Computer-Firmware#293 dealing with Telemetry and State Estimations to consolidate testing effort.

Issue Link

No subtasks linked to driver.

Parent: SunDevilRocketry/Flight-Computer-Firmware#301

Testing

  • Passes existing unit tests
  • Unit tests modified
  • Integration test performed

Test fails in CI are due to an inability to check out the appropriate parent ref. These issues are fixed in the flight-computer-firmware version of the driver runner.

Other

To be cross-reviewed by @ETSells and @NArmistead

Reviewer Checklist

Standards

  • Follows FCF Architectural Standards
  • Follows SDR Coding Standards
  • Code complexity/function Size is minimized
  • Code is testable
  • Code is readable and commented properly
  • License terms are respected

Error Handling

  • Potentially unsafe functions return a status code
  • Error returns properly handled

Memory

  • Stack allocated memory is scoped correctly
  • Heap allocated memory is avoided
  • Globally allocated memory is minimized except when necessary
  • Pointers are used correctly
  • Concurrency has been considered

Performance

  • Rate limiters are respected
  • Busy waiting is avoided
  • "Delay" calls are not used in performance sensitive code

@ETSells
ETSells marked this pull request as ready for review July 11, 2026 19:32

@ETSells ETSells left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

stamp. good to go imho. telem side needs review

@NArmistead NArmistead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Honestly this looks good and I don't really have much to say. maybe see if @266-750Balloons can also take a look since I don't know a ton about lora

Comment thread lora/lora_async.c
Comment on lines +34 to +40
#include "led.h"
#elif defined( GROUND_STATION )
#include "sdr_pin_defines_A0005.h"
#include "led.h"
#elif defined( A0010 )
#include "sdr_pin_defines_A0010.h"
#include "led.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: led.h include could go under project includes

Comment thread lora/lora_async.c
#include "main.h"

/*------------------------------------------------------------------------------
Global Variables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: these are all static variables but I don’t think we have a standard for what goes under “static variables” vs “global variables”

Comment thread lora/lora_async.c
Comment on lines +148 to +151
// ETS TEMP: Test
// telemetry_get_next_message();
// lora_transmit( &payload, sizeof( TELEMETRY_MESSAGE ) );
// return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are you keeping this?

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.

3 participants