Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tyler Chen Onboarding #163

Open
wants to merge 4 commits into
base: level3
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion lm75bd/lm75bd.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
/* LM75BD Registers (p.8) */
#define LM75BD_REG_CONF 0x01U /* Configuration Register (R/W) */


error_code_t lm75bdInit(lm75bd_config_t *config) {
error_code_t errCode;

Expand All @@ -25,9 +26,22 @@ error_code_t lm75bdInit(lm75bd_config_t *config) {
return ERR_CODE_SUCCESS;
}

#define CONF_WRITE_POINTER_BUFF_SIZE 1U
error_code_t readTempLM75BD(uint8_t devAddr, float *temp) {

Choose a reason for hiding this comment

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

Check that the temp is not null before use

/* Implement this driver function */

error_code_t errCode;
uint8_t buff[CONF_WRITE_POINTER_BUFF_SIZE] = {0};
errCode = i2cSendTo(devAddr, buff, CONF_WRITE_POINTER_BUFF_SIZE);
if (errCode != ERR_CODE_SUCCESS) return errCode;
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use RETURN_IF_ERROR_CODE to simplify this

Comment on lines +34 to +35

Choose a reason for hiding this comment

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

Better to use RETURN_IF_ERROR_CODE macro instead


uint8_t temp_bytes[2U] = {0}; // first interpret data as unsigned

Choose a reason for hiding this comment

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

If you are going to define a macro for the length above, probably best to define a macro here as well

errCode = i2cReceiveFrom(devAddr, temp_bytes, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

use sizeof(temp_bytes) rather than hardcoding 2

Choose a reason for hiding this comment

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

Use sizeof(temp_bytes) instead of 2. Also you should use the naming convention of camelCase

if (errCode != ERR_CODE_SUCCESS) return errCode;
Comment on lines +38 to +39

Choose a reason for hiding this comment

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

Better to use the RETURN_IF_ERROR_CODE macro instead


int data = temp_bytes[0];
data -= 2 * ((1 << 7) & data); // handle signed bit
data = (data << 3) | (temp_bytes[1] >> 5); // get LSbyte
Comment on lines +41 to +43

Choose a reason for hiding this comment

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

This code can be simplified to int16_t data = ( (tempBytes[0] << 8) | tempBytes[1]) >> 5;

*temp = (float) data * 0.125;
return ERR_CODE_SUCCESS;
}

Expand Down
2 changes: 1 addition & 1 deletion lm75bd/lm75bd.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <stdint.h>

/* LM75BD I2C Device Address */
#define LM75BD_OBC_I2C_ADDR /* Define the address here */
#define LM75BD_OBC_I2C_ADDR 0x4FU

/* LM75BD Configuration Values */
#define LM75BD_DEV_OP_MODE_NORMAL 0x00U
Expand Down
28 changes: 25 additions & 3 deletions services/thermal_mgr/thermal_mgr.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ static StackType_t thermalMgrTaskStack[THERMAL_MGR_STACK_SIZE];

#define THERMAL_MGR_QUEUE_LENGTH 10U
#define THERMAL_MGR_QUEUE_ITEM_SIZE sizeof(thermal_mgr_event_t)
#define THERMAL_OVERTEMPERATURE_THRESHOLD 80.0
#define THERMAL_HYSTERESIS_THRESHOLD 75.0

static QueueHandle_t thermalMgrQueueHandle;
static StaticQueue_t thermalMgrQueueBuffer;
Expand All @@ -38,23 +40,43 @@ void initThermalSystemManager(lm75bd_config_t *config) {
thermalMgrQueueHandle = xQueueCreateStatic(
THERMAL_MGR_QUEUE_LENGTH, THERMAL_MGR_QUEUE_ITEM_SIZE,
thermalMgrQueueStorageArea, &thermalMgrQueueBuffer);

}

error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) {
/* Send an event to the thermal manager queue */

Copy link
Contributor

Choose a reason for hiding this comment

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

check to make sure event is not NULL and that the queue has been created already

xQueueSend(thermalMgrQueueHandle, event, portMAX_DELAY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Always check the return value for non-void functions

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since you are calling this function in an ISR, the block time should be 0 since you should never be blocking in an ISR

Choose a reason for hiding this comment

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

Add error handling for this:

  • check that the event is not null, return invalid argument otherwise
  • check that the thermalMgrQueueHandle is not null, otherwise return invalid state
  • check the return value of the xQueueSend

Also this should be 0 and not PORTMAX_DELAY as this will block the task which might not be the intended behavior especially in ISR

return ERR_CODE_SUCCESS;
}

void osHandlerLM75BD(void) {
/* Implement this function */
thermal_mgr_event_t event;
event.type = THERMAL_MGR_EVENT_OS_INTERRUPT;
thermalMgrSendEvent(&event);
}

static void thermalMgr(void *pvParameters) {
/* Implement this task */
while (1) {

thermal_mgr_event_t event;
if (xQueueReceive(thermalMgrQueueHandle, &event, portMAX_DELAY)) {

Choose a reason for hiding this comment

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

Explicitly check against pdTRUE

const lm75bd_config_t data = *(lm75bd_config_t *) pvParameters;

Choose a reason for hiding this comment

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

This should be outside of the loop so we don't constantly re create it the data and better to use a pointer instead of a regular struct

float temperature;
readTempLM75BD(data.devAddr, &temperature);

Choose a reason for hiding this comment

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

In general, you should be checking the return of non-void functions for error codes. This should be wrapped inside of a LOG_IF_ERROR_CODE and if(errCode!= success) continue;
Also this should be moved to each event that needs it. You don't want to read the temp on all events

switch (event.type) {
case THERMAL_MGR_EVENT_MEASURE_TEMP_CMD:
addTemperatureTelemetry(temperature);
break;
case THERMAL_MGR_EVENT_OS_INTERRUPT:
if (temperature > THERMAL_OVERTEMPERATURE_THRESHOLD) {
overTemperatureDetected();
}
if (temperature < THERMAL_HYSTERESIS_THRESHOLD) {
safeOperatingConditions();
}
break;
}
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion services/thermal_mgr/thermal_mgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@

typedef enum {
THERMAL_MGR_EVENT_MEASURE_TEMP_CMD,

THERMAL_MGR_EVENT_OS_INTERRUPT,
} thermal_mgr_event_type_t;


typedef struct {
thermal_mgr_event_type_t type;
} thermal_mgr_event_t;
Expand Down
Loading