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

[FWXV 246] Added Files for Review #242

Merged
merged 2 commits into from
Jan 6, 2024

Conversation

bkctrl
Copy link
Member

@bkctrl bkctrl commented Dec 2, 2023

Added files for review for my first task, please let me know of any issues/points to fix!

@bkctrl bkctrl changed the title Added Files for Review [FWXV 246] Added Files for Review Dec 2, 2023
@bkctrl bkctrl requested a review from mitchellostler December 3, 2023 16:03
bps_indicator.h Outdated
@@ -0,0 +1,15 @@
#ifndef BPS_INDICATOR_H
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use #pragma once instead of #ifndef for our header files

bps_indicator.h Outdated
#include "outputs.h"

// callback function to periodically toggle the state of the bps fault indicator
static void prv_callback();
Copy link
Collaborator

Choose a reason for hiding this comment

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

static functions should not be in header files.

bps_indicator.h Outdated
static void prv_callback();

// functions to start/stop bps fault indicator
static void start_bps_fault_indicator();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of leaving the brackets empty in these methods, it's better to use the void keyword. We also need to remove the static keyword here.

bps_indicator.c Outdated

static void stop_bps_fault_indicator() {
s_bps_fault_enabled = false;
soft_timer_cancel(&s_bps_indicator_timer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: fix the indent.

Also, we don't need this cancel method, since we want the callback to be called a final time to turn off the hazard lights

bps_indicator.c Show resolved Hide resolved
@bkctrl bkctrl requested a review from mitchellostler January 6, 2024 13:34
@mitchellostler mitchellostler merged commit 9e2b3dc into main Jan 6, 2024
1 check passed
@mitchellostler mitchellostler deleted the FWXV-246-add-bps-strobe-functionality-to-pd branch January 6, 2024 21:01
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.

2 participants