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 fw 103 homework #489

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShahriarAhnaf
Copy link

please review for accuracy.

Copy link
Contributor

@ShiCheng-Lu ShiCheng-Lu left a comment

Choose a reason for hiding this comment

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

There are 2 errors that would stop this project from doing what is intended, but apart from that, there are only minor problems.

you should run formatting and linting before you push. If you've set up the github hooks, it should format and lint before every commit.

your branch name should be soft_999_... with a underscore.

you can move on to 104 after fixing the errors, and probably take on a ticket if you are interested.

interrupt_init();
soft_timer_init();
gpio_init();

Copy link
Contributor

Choose a reason for hiding this comment

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

gpio_it_init() needs to be called to enable interrupts from gpio pins

} else {
LOG_DEBUG("Failed to read potentiomter data");
}
delay_ms(200);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this delay for?

.priority = INTERRUPT_PRIORITY_NORMAL, //
};

gpio_init_pin(&potentiometer_addr, &pot_settings);
Copy link
Contributor

Choose a reason for hiding this comment

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

you also need to init the button pin to use it

.pin = 6,
};
// function to call when the button gets hit
static void button_interrupt_handler(const GpioAddress *addy, void *context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we prefix static methods with prv_ and global static variables with s_.

(addy is an interesting variable name, typo?)

INTERRUPT_EDGE_FALLING,
&button_interrupt_handler, // what function to call
&potentiometer_addr // what to send the function
);
Copy link
Contributor

Choose a reason for hiding this comment

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

here you are not using the void * context in your function anyway so its ok to pass in NULL.

format is also complaining about this ending parenthesis, I would put it straight after the last argument.

&potentiometer_addr // what to send the function
);
while (true) {
// let cpu run wild until interrupt.
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add a wait() from wait.h here to save on processing time.


int main(void) {
interrupt_init();
soft_timer_init();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think soft timer is used in this project, you don't need to include and init it.

Copy link
Contributor

@mitchellostler mitchellostler left a comment

Choose a reason for hiding this comment

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

Take a look at and address Shicheng's requested changes when you get the chance then we can move you on to a task

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