-
Notifications
You must be signed in to change notification settings - Fork 18
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
ISR Logging #253
base: main
Are you sure you want to change the base?
ISR Logging #253
Conversation
Pull reviewers statsStats of the last 120 days for UWOrbital:
|
Discussed in person; changes were unwanted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code duplicates calls to send event and semaphore give which might not be intended
Changes addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some previously requested changes not addressed
xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken); | ||
if (xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken) != pdTRUE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would send an event twice to the queue. This is not the intended behaviour. Remove the first call
xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken); | ||
|
||
if (xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken) == pdTRUE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would send an event twice to the queue. This is not the intended behaviour. Remove the first call
@@ -153,6 +154,12 @@ void i2cNotification(i2cBASE_t *i2c, uint32 flags) { | |||
|
|||
if (flags & I2C_SCD_INT) { | |||
xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken); | |||
|
|||
if (xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken) == pdTRUE) { | |||
LOG_INFO_FROM_ISR("Semaphore successfully given."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only log on failures so remove this call and only include the else case
@@ -103,6 +103,7 @@ obc_error_code_t i2cReceiveFrom(uint8_t sAddr, uint16_t size, uint8_t *buf, Tick | |||
|
|||
if (xSemaphoreTake(i2cTransferComplete, transferTimeoutTicks) != pdTRUE) { | |||
errCode = OBC_ERR_CODE_I2C_TRANSFER_TIMEOUT; | |||
LOG_ERROR_FROM_ISR("Semaphore cannot be given or was already given."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just log error code using the error code on line 105
@@ -224,6 +224,9 @@ void alarmInterruptCallback(void) { | |||
|
|||
alarm_handler_event_t event = {.id = ALARM_HANDLER_ALARM_TRIGGERED}; | |||
xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken); | |||
if (xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken) != pdTRUE) { | |||
LOG_ERROR_FROM_ISR("Alarm callback failed."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt an alarm callback. This is just an event being sent to the queue that failed. So probably log queue full
Purpose
Notion task: https://www.notion.so/uworbital/6375c5c5dcf1485e8a6c47f18d419ae5?v=72baf90d01f643b09d77418bd3f53b14&p=0a4d10df89ca420d899fc3c701b027d0&pm=s
Added logging to appropriate ISRs
New Changes
obc_logging.h
in ISRs that are both flagged with__attribute__(weak)
and have an existing definition.Testing
Outstanding Changes