Added new Thermal Manager with more temperature telemetry (besides lm75bd) #657

AnshRoutray · 2026-01-20T01:38:49Z

Purpose

Closes #285.
Explain the purpose of the PR here if it doesn't match the linked issue. Be sure to add a comment in the linked issue explaining the changes.

New Changes

Explain new changes below in short bullet points.

Replaced the entire Health Collector Module with Thermal Manager.
Added a statically allocated mailbox queue to ensure thermal manager can read temperature values from multiple sensors without directly accessing the drivers.
Added temperature input from RTC and CC1120 (currently commented out) apart from just lm75bd.

Testing

Explain tests that you ran to verify code functionality.

[ X] I have unit-tested this PR. Otherwise, explain why it cannot be unit-tested.
[ X] I have tested this PR on a board if the code will run on a board (Only required for firmware developers).
I have tested this PR by running the ARO website (Only required if the code will impact the ARO website).
I have tested this PR by running the MCC website (Only required if the code will impact the MCC website).
[ X] I have included screenshots of the tests performed below.

Outstanding Changes

If there are non-critical changes (i.e. additional features) that can be made to this feature in the future, indicate them here.

Note: The Comms manager task is not working properly (as indicated in a draft PR Draft PR for Comms Manager task being suspended issue. #632). I have commented out the cc1120 code for now, once the comms manager issue is fixed, the code should be uncommented out.

Output logs

…until the task suspension issue is solved.

github-actions · 2026-01-20T01:39:05Z

Pull reviewers stats

Stats of the last 120 days for UWOrbital:

User	Total reviews	Time to review	Total comments
Adityya-K 🥇	37 ▀▀▀▀	3d 35m ▀	107 ▀▀▀
camspec 🥈	34 ▀▀▀	3d 3h 23m ▀	191 ▀▀▀▀▀
proprogrammer504 🥉	13 ▀	7d 1h 30m ▀▀	44 ▀
Syzygicality	8 ▀	8h 20m	17
kepler452b123	5	1d 11h 18m	2
joannalauu	3	4d 21h 20m ▀	19
c4bae	1	13d 1h 35m ▀▀▀▀	2
panthpatel2016	1	4h 30m	6

^{⚡️ Pull request stats}

camspec

Great work overall, but there a few issues throughout

camspec · 2026-01-23T03:53:27Z

obc/app/modules/comms_link_mgr/comms_manager.c

 }

+static obc_error_code_t postCommsManagerTempQueue() {
+  float value = 0.0f;  // dummy value, replace with actual temp reading function


Label this as a TODO: and add it to outstanding changes

camspec · 2026-01-23T03:54:24Z

obc/app/modules/comms_link_mgr/comms_manager.h

 */
 obc_error_code_t sendToCommsManagerQueue(comms_event_t *event);

+/**


I recognize that this declaration is commented out due to what is mentioned in the PR description, but you also have the same declaration written below, uncommented

camspec · 2026-01-23T03:59:42Z

obc/app/modules/timekeeper/timekeeper.c

-    // Send Unix time to fram
+    // Post temperature into mailbox queue
+
+    postRtcTempQueue();


Ignoring the return value of this function, which is an error code?

camspec · 2026-01-23T04:00:52Z

obc/app/modules/timekeeper/timekeeper.c

+static obc_error_code_t postRtcTempQueue();
+
+void obcTaskInitTimekeeper(void) {
+  ASSERT((rtcTempQueueStack != NULL) && (&rtcTempQueue != NULL));


Taking the address of a static variable like rtcTempQueue will never result in NULL

camspec · 2026-01-23T04:07:04Z

obc/app/modules/timekeeper/timekeeper.c


-void obcTaskInitTimekeeper(void) {}
+#define RTC_TEMP_QUEUE_LENGTH 1
+#define RTC_TEMP_QUEUE_ITEM_SIZE sizeof(uint32_t)


Even though we can assume floats on our architecture are 32 bit, a type mismatch like this is unideal and not portable. We should define the size as being the same as the type of data we store in the queue.
Separate from this task, the driver function should probably be written to read this value as a fixed-point value rather than a floating-point number, which would avoid floats altogether

camspec · 2026-01-23T04:09:40Z

obc/app/modules/thermal_mgr/thermal_mgr.c

+  /* Uncomment this if comms manager task being suspened issue is fixed.
+  float cc1120Temp = 0.0f;
+  RETURN_IF_ERROR_CODE(readCC1120Temp(&cc1120Temp));
+  telemetry_data_t cc1120TempVal = {.obcTemp = lm75bdTemp, .id = TELEM_OBC_TEMP, .timestamp = getCurrentUnixTime()};


This code is commented out, but this should not be lm75bdTemp

camspec · 2026-01-23T04:10:15Z

obc/app/modules/thermal_mgr/thermal_mgr.c

+
+  float lm75bdTemp = 0.0f;
+  RETURN_IF_ERROR_CODE(readTempLM75BD(LM75BD_OBC_I2C_ADDR, &lm75bdTemp));
+  telemetry_data_t lm75bdTempVal = {.obcTemp = lm75bdTemp, .id = TELEM_OBC_TEMP, .timestamp = getCurrentUnixTime()};


Why do all three temp structs have the same id?

camspec · 2026-01-23T04:11:31Z

obc/app/modules/thermal_mgr/thermal_mgr.c

+
+  float lm75bdTemp = 0.0f;
+  RETURN_IF_ERROR_CODE(readTempLM75BD(LM75BD_OBC_I2C_ADDR, &lm75bdTemp));
+  telemetry_data_t lm75bdTempVal = {.obcTemp = lm75bdTemp, .id = TELEM_OBC_TEMP, .timestamp = getCurrentUnixTime()};


You call getCurrentUnixTime() separately for each temp value, but this can cause a race condition if time increases between these calls and we may get a discrepancy

camspec · 2026-01-23T04:12:45Z

obc/app/modules/comms_link_mgr/comms_manager.c

                                                      0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F};

+#define CC1120_TEMP_QUEUE_LENGTH 1
+#define CC1120_TEMP_QUEUE_ITEM_SIZE sizeof(uint32_t)


Same type mismatch as mentioned in the other comment

camspec · 2026-01-23T04:13:38Z

obc/app/modules/comms_link_mgr/comms_manager.c

+
+QueueHandle_t cc1120TempQueueHandle = NULL;
+static StaticQueue_t cc1120TempQueue;
+static uint8_t cc1120TempQueueStack[COMMS_MANAGER_QUEUE_LENGTH * CC1120_TEMP_QUEUE_ITEM_SIZE];


Why are you using the queue length of the comms manager here?

AnshRoutray and others added 9 commits September 23, 2025 21:25

Finished the thermal manager implementation for Comms, and RTC

14c0ae0

Completed the thermal manager taks, testing required

1799f4f

Merge branch 'main' into AnshumanRoutray/ThermalMgr

a9b70c7

Added the Posting Into Mailbox feature

7595058

timekeeper code working, however comms_manager code is commented out …

932bc71

…until the task suspension issue is solved.

Moved the readTempFunctions from thermal mgr to their respective files

52d9aa2

Fixing a pre-commit issue

c7ceed5

Added header definition for readCC1120Temp(float* temp)

a87b930

Added telemtry data for cc1120 temp and rtcTemp.

307e879

camspec requested changes Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new Thermal Manager with more temperature telemetry (besides lm75bd) #657

Added new Thermal Manager with more temperature telemetry (besides lm75bd) #657

Uh oh!

AnshRoutray commented Jan 20, 2026 •

edited by camspec

Loading

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

camspec left a comment

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

camspec Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Added new Thermal Manager with more temperature telemetry (besides lm75bd) #657

Are you sure you want to change the base?

Added new Thermal Manager with more temperature telemetry (besides lm75bd) #657

Uh oh!

Conversation

AnshRoutray commented Jan 20, 2026 • edited by camspec Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

New Changes

Testing

Outstanding Changes

Uh oh!

github-actions bot commented Jan 20, 2026

Pull reviewers stats

Uh oh!

camspec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AnshRoutray commented Jan 20, 2026 •

edited by camspec

Loading