- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2
Rudder control model #33
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
base: rudder-working-branch
Are you sure you want to change the base?
Conversation
Signed-off-by: ELEC_lead-ubcsailbot <[email protected]>
Signed-off-by: ELEC_lead-ubcsailbot <[email protected]>
Signed-off-by: ELEC_lead-ubcsailbot <[email protected]>
Signed-off-by: ELEC_lead-ubcsailbot <[email protected]>
|  | ||
| void updateAverages(PIDController controller) { | ||
| // Simple moving average for linear velocity, angular velocity, and heading | ||
| static linVelocityBuffer[10] = {0}; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a general comment, but I recommend putting this into a c compiler sooner than later, even if you aren't running the actual model in it. I don't think the static checker is super thorough, for example, this line I'm pretty sure won't compile because you aren't defining the type. Better to fix this before it becomes a massive headache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also want to define the 10 in a macro instead so that we do have some ability to change it
And you could consider having one function for calculating a rolling average with an array and the new value as parameters, which might reduce code repetition.
| PIDController controller = initController(fixedController, liveController); | ||
| } | ||
|  | ||
| void updateControllerVariables (PIDController controller) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any good thoughts on this yet, but the wind sensor, IMU, and desired heading will get updated whenever the sensors feel like it. It might make more sense to have three different functions for the IMU, windsensor, and desired heading, where the user passes the new data as parameters. This does make your code initialization a little more annoying, though.
Also, I might just have neglected to tell you this (apologies if I did), but we have two wind sensors, one on the top of the mast and one on the hull. A simple average would probably be a good first start, but it might be good to consider something more advanced in the future that could be robust to losing one windsensor, or if we start getting bad data from one or something). Although that being said for next on water test we will just have one installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, one other annoying thing I don't know the exact consequences of: different sensors will update at different frequencies. Like the windsensors are 4Hz, the IMU will in the future be something like 100Hz
| // Low pass filtering the derivative | ||
| cState.filteredError = PID.derivativeFilterFactor * currentError + (1 - PID.derivativeFilterFactor) * cState.previousFilteredError; | ||
| // Calculating the derivative | ||
| float derivative = (cState.filteredError - cState.previousFilteredError) / dt; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible / 0 error here. You don't need to handle this in a nice way, but we don't want to controller hard faulting on some freak incident or something.
No description provided.