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.

Sign up for GitHub

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

Jump to bottom

Implement Blocking Logs #252

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

Implement Blocking Logs #252

wants to merge 6 commits into from

Conversation

ShourryaGuha
Copy link
Contributor

Purpose

Add blocking logs feature

New Changes

  • Added a blocking log macro which when defined at compile time changes the LOGGER_QUEUE_TX_WAIT_PERIOD to portMAX_DELAY
  • This mode ensures that no log message is lost

Testing

  • Ran unit tests and they succeeded

Outstanding Changes

  • If there are non-critical changes (i.e. additional features) that can be made to this feature in the future, indicate them here.

@github-actions GitHub Actions
Copy link

Pull reviewers stats

Stats of the last 120 days for UWOrbital:

User Total reviews Time to review Total comments
Navtajh04 51 3d 17h 36m 541
dgobalak 25 8d 3h 56m 258
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

Navtajh04
Navtajh04 requested changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@Navtajh04 Navtajh04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple of quick changes looks good otherwise

cmake/obc_build_options.cmake Outdated Show resolved Hide resolved
obc/modules/logger/logger.c Outdated Show resolved Hide resolved
obc/modules/logger/logger.c Outdated Show resolved Hide resolved
obc/modules/logger/logger.c Outdated Show resolved Hide resolved
@ShourryaGuha
Copy link
Contributor Author

Addressed requested changes @Navtajh04

dgobalak
dgobalak requested changes Nov 30, 2023
View reviewed changes
Copy link
Member

@dgobalak dgobalak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be tested on a board before merge. Our unit tests don't cover this

obc/CMakeLists.txt Outdated Show resolved Hide resolved
@ShourryaGuha
Copy link
Contributor Author

This should be tested on a board before merge. Our unit tests don't cover this

@dgobalak so this is on hold till tested?

@dgobalak
Copy link
Member

dgobalak commented Dec 8, 2023

This should be tested on a board before merge. Our unit tests don't cover this

@dgobalak so this is on hold till tested?

ya we'll just wait for someone to test it before merge

Navtajh04
Navtajh04 reviewed Feb 5, 2024
View reviewed changes
Copy link
Contributor

@Navtajh04 Navtajh04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been tested on board yet? @ShourryaGuha

@ShourryaGuha ShourryaGuha force-pushed the cdh/shourrya/blocking-logs branch from 0fb0de2 to 2ad35a9 Compare June 14, 2024 04:28
dgobalak
dgobalak reviewed Jun 26, 2024
View reviewed changes
obc/app/modules/logger/logger.c
@@ -31,7 +31,12 @@ static log_output_location_t outputLocation;
#define LOGGER_QUEUE_LENGTH 10U
#define LOGGER_QUEUE_ITEM_SIZE sizeof(logger_event_t)
#define LOGGER_QUEUE_RX_WAIT_PERIOD portMAX_DELAY

#ifdef ENABLE_BLOCKING_LOGS
#define LOGGER_QUEUE_TX_WAIT_PERIOD portMAX_DELAY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Navtajh04 This is just meant for dev purposes right? Cuz on release version, the risk of deadlock and significant delays (causing watchdog timeouts and poor performance) is prob too much

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

@Yarik-Popov Yarik-Popov changed the title Cdh/shourrya/blocking logs Implement Blocking Logs Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Navtajh04 Navtajh04 Navtajh04 requested changes

@dgobalak dgobalak dgobalak requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShourryaGuha @dgobalak @Navtajh04