search

sfuphantom / hercules-phantom-lib

A common firmware library for all Team Phantom PCBs based off of the TMS570LS1224/1227 Hercules line
0 stars 0 forks source link

Phal drivers #13

Closed ManojBR105 closed 8 months ago

ManojBR105 commented 9 months ago

Purpose

Uart Drivers Completed needs code review before accepting This file will standardize information needed for a pull request

Implementation

This file uses markdown to format the text into headers

Testing

View on github

References

https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#referencing-issues-and-pull-requests

PR Review Rules

  1. Reviewer is responsible for resolving any threads they started
  2. One commit per thread and if it's a widespread change, link the commit in the reply pls :)
  3. ...
rafguevara14 commented 9 months ago

The HAL folder should be inside the PHAL folder, because it is the only folder that will need it

rafguevara14 commented 9 months ago

A new folder in the HAL folder containing... readme.md w/ setup instructions, code snippets, and verification documentation Header and implementation files

The source and header files should be in one folder

rafguevara14 commented 9 months ago

So my understanding is that any C file that includes this C++ file will need to either be a C++ file or have the #ifdef statements at the top.

For the BMS, I think they already have a lot to worry about so going through and doing that for every file in their codebase my not be ideal. I'd like their team to just be able to include our files with no issues.

I will review the C changes for now

ManojBR105 commented 9 months ago

The HAL folder should be inside the PHAL folder, because it is the only folder that will need it

done

ManojBR105 commented 9 months ago

Looking good so far! I think we're closing in on a good API here. Most of the functions look great. Just a few comments about some of the other functions.

Also, can we keep to one commit per thread that you are addressing (where possible ofc)? It will make it easier to go through the changes and speed up the review process. I'll re emphasize these PR rules again. I've had similar rules at my embedded software coops and they've helped a lot.

- Reviewer is responsible for resolving any threads they started
- One commit per thread and if it's a widespread change, link the commit in the reply pls :)
...

Oh sorry, my bad.... will follow it in the future