lhr-solar / BPS

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

update pid loop #561

Closed MichaelMohn closed 1 year ago

MichaelMohn 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

MichaelMohn commented 1 year ago

Change output to uint8_t if it only returns values from 0-8. I'm pretty sure your pid code works but instead of doing (-ErrorSum)/INTEGRAL*1000 do (-ErrorSum)*1000/INTEGRAL. The otherway kind of gets rid of precison I'm p sure.

Nah I might not understand this part of C perfectly... but at the moment ErrorSum is capped at 500,000 and integral is 250. So the max output is 2. If I place the 1000 in the numerator I would get a massively incorrect value right?

ray4477 commented 1 year ago

Change output to uint8_t if it only returns values from 0-8. I'm pretty sure your pid code works but instead of doing (-ErrorSum)/INTEGRAL*1000 do (-ErrorSum)*1000/INTEGRAL. The otherway kind of gets rid of precison I'm p sure.

Nah I might not understand this part of C perfectly... but at the moment ErrorSum is capped at 500,000 and integral is 250. So the max output is 2. If I place the 1000 in the numerator I would get a massively incorrect value right?

It should be equivalent mathematical statements. The problem with the before is if ErrorSum is say less than 250 than if it is divided by INTEGRAL (250) it will become zero because int division rounds down. If you do the other way then it will return fine. Here is an example below

image
MichaelMohn commented 1 year ago

Change output to uint8_t if it only returns values from 0-8. I'm pretty sure your pid code works but instead of doing (-ErrorSum)/INTEGRAL*1000 do (-ErrorSum)*1000/INTEGRAL. The otherway kind of gets rid of precison I'm p sure.

Nah I might not understand this part of C perfectly... but at the moment ErrorSum is capped at 500,000 and integral is 250. So the max output is 2. If I place the 1000 in the numerator I would get a massively incorrect value right?

It should be equivalent mathematical statements. The problem with the before is if ErrorSum is say less than 250 than if it is divided by INTEGRAL (250) it will become zero because int division rounds down. If you do the other way then it will return fine. Here is an example below image

This has been fixed I believe.