Code Review & Discussion#1
Conversation
| if (dev == NULL) { | ||
| /* No such node, or the node does not have status "okay". */ | ||
| LOG_WRN("No BME280 sensor found. Will be using simulator instead."); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!device_is_ready(dev)) { | ||
| LOG_ERR("Error: Device \"%s\" is not ready. Check the driver initialization logs for errors.", | ||
| dev->name); | ||
| return NULL; | ||
| } | ||
|
|
There was a problem hiding this comment.
For debug purposes it is okay but if the outcome is same for the upper modules, you can just use the if(NULL == dev && device_is_read(dev))
| const struct device *get_bme280_device(void) { | ||
| const struct device *const dev = DEVICE_DT_GET_ANY(bosch_bme280); | ||
|
|
||
| if (dev == NULL) { |
There was a problem hiding this comment.
When you are using an if against a macro, macro should be written first such as (NULL == dev), this eliminates the errors such as (dev = NULL).
There was a problem hiding this comment.
Need more info on the subject, I didn't get the point. Should we put the macro first, or last? Why would it be a problem to have macro output as NULL?
There was a problem hiding this comment.
When evaluating the equality of a variable against a constant, the constant should be on the left side. Such as,
if (NULL == dev)
This eliminates the errors such as accidently making the variable equal to the constant, which is a really hard to bug to catch.
if (dev = NULL) // This is a very hard bug to catch.
|
|
||
| while (1) { | ||
| // Wait for the semaphore to be available. | ||
| if (k_sem_take(&semaphore_send_ready, K_NO_WAIT) == 0) { |
There was a problem hiding this comment.
It is usually good option to hide the semaphores under a module or function that owns them, such as is_data_ready_to_send()
| notify_ble_connected_device(filtered_values); | ||
|
|
||
| // Free the memory. | ||
| k_free(filtered_values); |
There was a problem hiding this comment.
Using a static buffer would also eliminate the need for the freeing them because you were just going to overwrite the older values.
|
|
||
| void notify_ble_connected_device(struct values_for_ble_t *value_to_notify) { | ||
| // Save the int part and dec part of the reading into an array. | ||
| temp_reading = value_to_notify->temp_reading_int * 100 + value_to_notify->temp_reading_dec * 0.0001; |
There was a problem hiding this comment.
When there is some kind of conversion it is better to use function like macros for readbility and for elimating the duplication, such as,
| temp_reading = value_to_notify->temp_reading_int * 100 + value_to_notify->temp_reading_dec * 0.0001; | |
| temp_reading = SCALED_NUM_TO_INT(value_to_notify->temp_reading_int, value_to_notify->temp_reading_dec) |
| } | ||
|
|
||
|
|
||
| void notify_ble_connected_device(struct values_for_ble_t *value_to_notify) { |
There was a problem hiding this comment.
Should be const if not modified
| struct sensor_value_store_t { | ||
| void *lifo_reserved; | ||
| struct sensor_value temperature; | ||
| struct sensor_value pressure; | ||
| // struct sensor_value humidity; | ||
| }; No newline at end of file |
There was a problem hiding this comment.
You can use typedefs such as, the t suffix is for the typedef.
| struct sensor_value_store_t { | |
| void *lifo_reserved; | |
| struct sensor_value temperature; | |
| struct sensor_value pressure; | |
| // struct sensor_value humidity; | |
| }; | |
| typedef struct sensor_value_store_t { | |
| void *lifo_reserved; | |
| struct sensor_value temperature; | |
| struct sensor_value pressure; | |
| // struct sensor_value humidity; | |
| }; sensor_value_store_t; |
2 participants
This pull request is a draft PR that will be used for code-review and discussion for the life-time of the repo.