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

PedalsRead returning signed int #419

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

diyarajon
Copy link
Contributor

@diyarajon diyarajon commented Feb 29, 2024

Quality Assurance Checklist

Requirements

  • Followed Coding Style Guide
  • Presented/discussed in some capacity with others on the Controls team
  • Code Build checks pass
  • No merge conflicts
  • Software feature has documentation for it updated in both ReadTheDocs and the comments
  • Software feature has documentation for it updated
  • Testing
    • Software feature has test associated with it
    • Test provides useful information and uses relevant data to accurately represent Controls
    • Tested on hardware
    • NOTE: If test file already exists, use that one
  • If applicable, have you added the new feature to main.c?
  • Tagged appropriate issue ticket on this PR
  • Did you have fun?

Things to Consider

  • Even if the above are checked, is this the best way of writing my code?
    • It's possible to write code that works, but are there ways to make code more efficient?

@diyarajon diyarajon linked an issue Feb 29, 2024 that may be closed by this pull request
Copy link
Contributor

@IshDeshpa IshDeshpa left a comment

Choose a reason for hiding this comment

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

to test this I would just make sure that when the simboard is plugged in, both BSP_ADC and Pedals_Read return the right value. Write a test file that does this.

Drivers/Src/Pedals.c Show resolved Hide resolved
IshDeshpa
IshDeshpa previously approved these changes Feb 29, 2024
Copy link
Contributor

@IshDeshpa IshDeshpa left a comment

Choose a reason for hiding this comment

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

LGTM. @KnockbackNemo can you review?

KnockbackNemo
KnockbackNemo previously approved these changes Mar 1, 2024
Copy link
Contributor

@KnockbackNemo KnockbackNemo left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@diyarajon diyarajon dismissed stale reviews from KnockbackNemo and IshDeshpa via a9189ac March 2, 2024 19:48
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.

Pedals_Read returns a signed int
3 participants