Tyler Chen Onboarding #163
Conversation
| errCode = i2cSendTo(devAddr, buff, CONF_WRITE_POINTER_BUFF_SIZE); | ||
| if (errCode != ERR_CODE_SUCCESS) return errCode; |
There was a problem hiding this comment.
Better to use RETURN_IF_ERROR_CODE macro instead
| errCode = i2cReceiveFrom(devAddr, temp_bytes, 2); | ||
| if (errCode != ERR_CODE_SUCCESS) return errCode; |
There was a problem hiding this comment.
Better to use the RETURN_IF_ERROR_CODE macro instead
| if (errCode != ERR_CODE_SUCCESS) return errCode; | ||
|
|
||
| uint8_t temp_bytes[2U] = {0}; // first interpret data as unsigned | ||
| errCode = i2cReceiveFrom(devAddr, temp_bytes, 2); |
There was a problem hiding this comment.
Use sizeof(temp_bytes) instead of 2. Also you should use the naming convention of camelCase
| } | ||
|
|
||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | ||
| /* Send an event to the thermal manager queue */ | ||
|
|
||
| xQueueSend(thermalMgrQueueHandle, event, portMAX_DELAY); |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| static void thermalMgr(void *pvParameters) { | ||
| /* Implement this task */ | ||
| while (1) { | ||
|
|
||
| thermal_mgr_event_t event; | ||
| if (xQueueReceive(thermalMgrQueueHandle, &event, portMAX_DELAY)) { |
|
|
||
| thermal_mgr_event_t event; | ||
| if (xQueueReceive(thermalMgrQueueHandle, &event, portMAX_DELAY)) { | ||
| const lm75bd_config_t data = *(lm75bd_config_t *) pvParameters; |
There was a problem hiding this comment.
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
| if (xQueueReceive(thermalMgrQueueHandle, &event, portMAX_DELAY)) { | ||
| const lm75bd_config_t data = *(lm75bd_config_t *) pvParameters; | ||
| float temperature; | ||
| readTempLM75BD(data.devAddr, &temperature); |
There was a problem hiding this comment.
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
| int data = temp_bytes[0]; | ||
| data -= 2 * ((1 << 7) & data); // handle signed bit | ||
| data = (data << 3) | (temp_bytes[1] >> 5); // get LSbyte |
There was a problem hiding this comment.
This code can be simplified to int16_t data = ( (tempBytes[0] << 8) | tempBytes[1]) >> 5;
| errCode = i2cSendTo(devAddr, buff, CONF_WRITE_POINTER_BUFF_SIZE); | ||
| if (errCode != ERR_CODE_SUCCESS) return errCode; | ||
|
|
||
| uint8_t temp_bytes[2U] = {0}; // first interpret data as unsigned |
There was a problem hiding this comment.
If you are going to define a macro for the length above, probably best to define a macro here as well
| @@ -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) { | |||
There was a problem hiding this comment.
Check that the temp is not null before use
| errCode = i2cSendTo(devAddr, buff, CONF_WRITE_POINTER_BUFF_SIZE); | ||
| if (errCode != ERR_CODE_SUCCESS) return errCode; |
There was a problem hiding this comment.
You can use RETURN_IF_ERROR_CODE to simplify this
| if (errCode != ERR_CODE_SUCCESS) return errCode; | ||
|
|
||
| uint8_t temp_bytes[2U] = {0}; // first interpret data as unsigned | ||
| errCode = i2cReceiveFrom(devAddr, temp_bytes, 2); |
There was a problem hiding this comment.
use sizeof(temp_bytes) rather than hardcoding 2
| } | ||
|
|
||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | ||
| /* Send an event to the thermal manager queue */ | ||
|
|
There was a problem hiding this comment.
check to make sure event is not NULL and that the queue has been created already
| } | ||
|
|
||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | ||
| /* Send an event to the thermal manager queue */ | ||
|
|
||
| xQueueSend(thermalMgrQueueHandle, event, portMAX_DELAY); |
There was a problem hiding this comment.
Always check the return value for non-void functions
There was a problem hiding this comment.
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
No description provided.