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

Completed onboarding challenge #106

Closed
wants to merge 8 commits into from
Closed

Conversation

leozqi
Copy link

@leozqi leozqi commented Nov 15, 2023

Purpose

Pull Request for Firmware Onboarding Challenge.

Suggestions and tips appreciated!

New Changes

Implemented:

  • lm75bd.c readTempLM75BD function
  • thermal_mgr.c thermalMgr, osHandlerLM75BD functions

Testing

  • Passed temperature sensor unit tests
  • Integration tests: Over temperature detected entering 82 degC; safe-temperature resumption detected at 74 degC.

- Passed temperature sensor unit tests
- Integration tests: Over temperature detected entering 82 degC;
  safe-temperature resumption detected at 74 degC.

Implemented:

- lm75bd.c readTempLM75BD function
- thermal_mgr.c thermalMgr, osHandlerLM75BD functions
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.

Looks good, just a couple of changes

lm75bd/lm75bd.c Outdated Show resolved Hide resolved
lm75bd/lm75bd.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
- null checks for events and thermalMgrQueueHandle
- Modified thermalMgrSendEventISR to return ERR_CODE_QUEUE_FULL in event
  of xQueueSendFromISR fail
- Modified thermalMgrSendEvent to return ERR_CODE_QUEUE_FULL in event of
  xQueueSend fail
@leozqi leozqi force-pushed the level3 branch 2 times, most recently from 43e1ad8 to ca8eaba Compare November 22, 2023 17:41
- Correct pdTrue, pdFalse to pdTRUE, pdFALSE
@leozqi
Copy link
Author

leozqi commented Nov 22, 2023

Thanks for your feedback @Navtajh04 , appreciate it!

lm75bd/lm75bd.c Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
lm75bd/lm75bd.c Outdated Show resolved Hide resolved
lm75bd/lm75bd.c Outdated Show resolved Hide resolved
lm75bd/lm75bd.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
services/thermal_mgr/thermal_mgr.c Outdated Show resolved Hide resolved
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 1 change other than that looks good

}

float temp = {0};
RETURN_IF_ERROR_CODE(readTempLM75BD(config.devAddr, &temp));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be a log if there's an error code followed by a continue since we should not be returning from FreeRTOS functions and especially since this is a void function and this would be returning an error code

@dgobalak dgobalak closed this Jan 14, 2024
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.

3 participants