lhr-solar / BPS

Battery Protection System Code
MIT License
4 stars 2 forks source link

Add Pwm on simulator #519

Closed Champers5000 closed 1 year ago

Champers5000 commented 1 year ago

Quality Assurance Checklist

To make reviews more efficient, please make sure the software feature meets the following standards and check everything off that meets the quality check. Once everything has been checked, the assigned reviewers will begin the review process. Edit this description to check off the list.

There are exceptions with all guidelines. As long as your decisions are justified, then you are good! Contact the reviewers or the leads about any exceptions.

Requirements

Things to Consider

manthanand commented 1 year ago

make sure this just has pwm stuff and not test file stuff (unless it is necessary for it to work which I don't think it is).

manthanand commented 1 year ago

Also make sure documentation for both things are changed.

ray4477 commented 1 year ago

BSP_PWM replaces BSP_contactor correct? I think it's helpful to log if contactor is off or not if speed is set to 0 just to make log files easier to understand for now rather than just "set pin 3 to speed 0" or smth like that

manthanand commented 1 year ago

overall this looks good, but some files are marked as deleted on the bare metal folder (Contactor, Fans) and sim (also contactor and fans). I dont think these files should get thanos snapped unless I am just misunderstanding the file structure change with the PWM changes

The files can get Thanos'd cuz if we need them, we can just go to an earlier version of the master branch. I am against keeping old code in the repo because it clutters the repo. @connorl309

manthanand commented 1 year ago

BSP_PWM replaces BSP_contactor correct? I think it's helpful to log if contactor is off or not if speed is set to 0 just to make log files easier to understand for now rather than just "set pin 3 to speed 0" or smth like that

I also agree with this @Champers5000