Fatma J Onboarding #196
Fatma J Onboarding #196
Conversation
lm75bd/lm75bd.c
Outdated
| @@ -27,7 +27,17 @@ error_code_t lm75bdInit(lm75bd_config_t *config) { | |||
|
|
|||
| error_code_t readTempLM75BD(uint8_t devAddr, float *temp) { | |||
| /* Implement this driver function */ | |||
|
|
|||
| uint8_t sendBuf[1] = {0x0U}; | |||
| uint8_t buffer[2]; | |||
lm75bd/lm75bd.c
Outdated
| i2cSendTo (devAddr, sendBuf, 1U); | ||
| i2cReceiveFrom (devAddr, buffer, 2U); |
There was a problem hiding this comment.
- wrap both lines inside of a RETURN_IF_ERROR_CODE
- use sizeof(sendBuf) instead of 1
- use sizeof(buffer) instead of 2
services/thermal_mgr/thermal_mgr.c
Outdated
| @@ -43,18 +43,32 @@ void initThermalSystemManager(lm75bd_config_t *config) { | |||
|
|
|||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | |||
| /* Send an event to the thermal manager queue */ | |||
| xQueueSend(thermalMgrQueueHandle, event, 0); // why u throwing error | |||
There was a problem hiding this comment.
- check to make sure that thermal mgr queue isn't null before use, returning invalid state
- check to make sure that the event isn't null
- in general, you should be checking the return value of non void functions for errors and handling them accordingly
services/thermal_mgr/thermal_mgr.c
Outdated
| float temp; | ||
| readTempLM75BD(LM75BD_OBC_I2C_ADDR, &temp); | ||
|
|
||
| if(temp > 80) { | ||
| overTemperatureDetected(); | ||
| } else if(temp < 75) { | ||
| safeOperatingConditions(); | ||
| } |
There was a problem hiding this comment.
Do not read inside the os handler as it may block. Send an event and have the thermalMgr task function handle it
services/thermal_mgr/thermal_mgr.c
Outdated
| if(temp > 80) { | ||
| overTemperatureDetected(); | ||
| } else if(temp < 75) { |
There was a problem hiding this comment.
Use the macros defined in the lm75bd.h instead of hardocding the values
services/thermal_mgr/thermal_mgr.c
Outdated
| thermal_mgr_event_t data = *(thermal_mgr_event_t *) pvParameters; | ||
| if(xQueueReceive(thermalMgrQueueHandle, pvParameters, 0) == pdTRUE && data.type == THERMAL_MGR_EVENT_MEASURE_TEMP_CMD) { // ensure about the tick time + how to checl the type ajsdkfk | ||
| float temp; | ||
| readTempLM75BD(LM75BD_OBC_I2C_ADDR, &temp); |
There was a problem hiding this comment.
Check the return value for an error code, log it if it exists and continue. we don't want to add a temp value that failed to read we don't know if temp will be a valid value
services/thermal_mgr/thermal_mgr.c
Outdated
| } | ||
|
|
||
| static void thermalMgr(void *pvParameters) { | ||
| /* Implement this task */ | ||
| while (1) { | ||
|
|
||
| thermal_mgr_event_t data = *(thermal_mgr_event_t *) pvParameters; | ||
| if(xQueueReceive(thermalMgrQueueHandle, pvParameters, 0) == pdTRUE && data.type == THERMAL_MGR_EVENT_MEASURE_TEMP_CMD) { // ensure about the tick time + how to checl the type ajsdkfk |
There was a problem hiding this comment.
- Use portMAX_DELAY instead of 0
- you should be logging if an invalid event was received
lm75bd/lm75bd.c
Outdated
| uint8_t a = 1U; | ||
| uint8_t mask = a << 7; | ||
| if (!(buffer[0] & mask)) { | ||
| *temp = ((buffer[0] << 3) | (buffer[1] >> 5)) * 0.125; | ||
| } else { | ||
| *temp = -(((~((buffer[0] << 3) | (buffer[1] >> 5))) & (0b0000011111111111)) + 1) * 0.125; | ||
| } |
There was a problem hiding this comment.
the code is overly complicated
| uint8_t a = 1U; | |
| uint8_t mask = a << 7; | |
| if (!(buffer[0] & mask)) { | |
| *temp = ((buffer[0] << 3) | (buffer[1] >> 5)) * 0.125; | |
| } else { | |
| *temp = -(((~((buffer[0] << 3) | (buffer[1] >> 5))) & (0b0000011111111111)) + 1) * 0.125; | |
| } | |
| int16_t temperature = ((buffer[0] << 8) | buffer[1]) >> 5; | |
| *temp = temperature * 0.125; |
You might need to do some casting to get this to work
| @@ -43,18 +44,47 @@ void initThermalSystemManager(lm75bd_config_t *config) { | |||
|
|
|||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | |||
| /* Send an event to the thermal manager queue */ | |||
|
|
|||
| return ERR_CODE_SUCCESS; | |||
| if(thermalMgrQueueHandle != NULL && event != NULL) { | |||
There was a problem hiding this comment.
- return invalid argument if event is null
- return invalid state if thermal mgr queue handle is null as its not an argument but a state variable
|
|
||
| return ERR_CODE_SUCCESS; | ||
| if(thermalMgrQueueHandle != NULL && event != NULL) { | ||
| xQueueSend(thermalMgrQueueHandle, event, 0); |
There was a problem hiding this comment.
In general, you should check the return of non-void functions for error codes and handle them
| } | ||
|
|
||
| static void thermalMgr(void *pvParameters) { | ||
| /* Implement this task */ | ||
| while (1) { | ||
|
|
||
| thermal_mgr_event_t data = *(thermal_mgr_event_t *) pvParameters; |
There was a problem hiding this comment.
PvParameters is not of type thermal mgr event but of the lm75bd config so you shouldn't do this. Just thermal_mgr_event_t event={0};
|
|
||
| thermal_mgr_event_t data = *(thermal_mgr_event_t *) pvParameters; | ||
|
|
||
| error_code_t errCodeEvent = xQueueReceive(thermalMgrQueueHandle, pvParameters, portMAX_DELAY); |
There was a problem hiding this comment.
The return type of xQueueReceive isn't error_code_t. Only the functions we write on the team use that custom error code. Libraries have there own
| } | ||
| addTemperatureTelemetry(temp); | ||
| } else if(data.type == THERMAL_MGR_EVENT_OS_INTERRUPT) { | ||
| if(temp > LM75BD_DEFAULT_OT_THRESH) { |
There was a problem hiding this comment.
You should be reading the temperature when you get an over temperature event as if temp wasn't read for a while the value might be stale and not represent the current temperature
| uint8_t a = 1U; | ||
| uint8_t mask = a << 7; | ||
| int16_t temperature = ((buffer[0] << 8) | buffer[1]) >> 5; | ||
| if (!(buffer[0] & mask)) { | ||
| *temp = temperature * 0.125; | ||
| } else { | ||
| *temp = -((~temperature & 0b0000011111111111) + 1) * 0.125; | ||
| } |
There was a problem hiding this comment.
Out of these lines, you only need lines 40 and 42. You might need to cast the temperature before assign it to temp
Purpose
To check my onboarding
New Changes
Did onboarding
Testing
Looked at the test cases in the output
Outstanding Changes
Idk