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

ISR Logging #253

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

ISR Logging #253

wants to merge 8 commits into from

Conversation

guhansiyer
Copy link
Contributor

@guhansiyer guhansiyer commented Nov 29, 2023

Purpose

Notion task: https://www.notion.so/uworbital/6375c5c5dcf1485e8a6c47f18d419ae5?v=72baf90d01f643b09d77418bd3f53b14&p=0a4d10df89ca420d899fc3c701b027d0&pm=s

Added logging to appropriate ISRs

New Changes

  • Added logging with obc_logging.hin ISRs that are both flagged with __attribute__(weak) and have an existing definition.

Testing

  • Flashed image onto RM46

Outstanding Changes

Copy link

Pull reviewers stats

Stats of the last 120 days for UWOrbital:

User Total reviews Time to review Total comments
Navtajh04 52 3d 22h 20m 553
dgobalak 26 8d 21h 38m 268
manumanuk 7 5d 22m 22
Gjjjiang 7 20d 6h 13m 19
Yarik-Popov 5 4d 21h 14m 59
TheSpaceDragon 4 1d 18h 27m 0
PratyushMakkar 1 8d 16h 7m 2

obc/drivers/rm46/obc_gio_ctrl.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_gio_ctrl.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_i2c_io.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_gio_ctrl.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_i2c_io.c Outdated Show resolved Hide resolved
@guhansiyer guhansiyer dismissed stale reviews from dgobalak and Navtajh04 February 5, 2024 19:49

Discussed in person; changes were unwanted

obc/drivers/rm46/obc_i2c_io.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_i2c_io.c Outdated Show resolved Hide resolved
obc/modules/alarm_mgr/alarm_handler.c Outdated Show resolved Hide resolved
obc/modules/comms_link_mgr/cc1120_txrx.c Outdated Show resolved Hide resolved
obc/modules/comms_link_mgr/cc1120_txrx.c Outdated Show resolved Hide resolved
obc/modules/comms_link_mgr/cc1120_txrx.c Outdated Show resolved Hide resolved
obc/modules/comms_link_mgr/cc1120_txrx.c Outdated Show resolved Hide resolved
Copy link
Contributor

@Yarik-Popov Yarik-Popov left a 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

obc/drivers/rm46/obc_i2c_io.c Outdated Show resolved Hide resolved
obc/modules/alarm_mgr/alarm_handler.c Outdated Show resolved Hide resolved
obc/drivers/rm46/obc_gio_ctrl.c Outdated Show resolved Hide resolved
@guhansiyer guhansiyer dismissed stale reviews from Yarik-Popov and Navtajh04 February 1, 2025 03:44

Changes addressed

Copy link
Contributor

@Yarik-Popov Yarik-Popov left a 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

Comment on lines 226 to +227
xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken);
if (xQueueSendToFrontFromISR(alarmHandlerQueueHandle, (void *)&event, &xHigherPriorityTaskWoken) != pdTRUE) {
Copy link
Contributor

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

Comment on lines 156 to +158
xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken);

if (xSemaphoreGiveFromISR(i2cTransferComplete, &xHigherPriorityTaskWoken) == pdTRUE) {
Copy link
Contributor

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.");
Copy link
Contributor

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.");
Copy link
Contributor

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.");
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants