Firmware Training- Rukshi Thiyagayogan #380
Conversation
There was a problem hiding this comment.
Hello I reviewed your servo code, so far you've done a good job usually it takes many reviews before code gets approved so don't be discouraged by the number of comments. If anything is unclear don't hesitate to reach out and ask. You can message me on the discord or reply to one of my comments.
| using namespace std; | ||
|
|
||
|
|
||
| TutorialServo::TutorialServo(PinName servoPin, float servoRangeInDegrees = 180.0, |
There was a problem hiding this comment.
instead of providing default values as you have done we do it by providing something called a member initializer list. Below is a link explaining what that means. Try implementing this in your code.
|
|
||
| #include "mbed.h" | ||
| #include "TutorialServo.h" | ||
|
|
There was a problem hiding this comment.
The only thing you need to include is TutorialServo.h, also we dont normally use "using namespace std"
| TutorialServo::TutorialServo(PinName servoPin, float servoRangeInDegrees = 180.0, | ||
| float minPulsewidthInMs = 1, float maxPulsewidthInMs = 2) | ||
| { | ||
| //declares this pin as pwm using pwmout. |
There was a problem hiding this comment.
Inside your constructor there doesn't need to be any code. Really in this application the only purpose of the constructor is to use the member initializer list to initialize the member variables of the object.
| void TutorialServo::setPositionInDegrees(const float degrees) | ||
| { | ||
|
|
||
| m_servoRangeInDegrees = degrees; |
There was a problem hiding this comment.
What you are doing here is changing the member variable, this isn't going to change the angle of the motor at all. What you want to do is to call the method of setting the pulsewidth on the servoPwmOut i.e including the following line of code instead:
m_servoPwmOut.pulsewidth_ms(some_argument);
Think about what should the argument for this function be and dont hesitate to hit me up on discord for more help.
| float TutorialServo::getServoRangeInDegrees() const | ||
| { | ||
| return m_servoRangeInDegrees; | ||
| } |
| float TutorialServo::getMaxPulseWidthInMs() const | ||
| { | ||
| return m_maxPulsewidthInMs; | ||
| } |
| @@ -0,0 +1,24 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
check your indenting on this file, depending on your code editor you can probably select a setting that can format your code every time you save. If that's not an option make sure you go through and manually indent.
There was a problem hiding this comment.
There's a section in our repo readme called "Clang-Format" that walks you through setting up the formatter we use. Please go through it and use it to format the training files. Our formatting style uses an indentation of 2 spaces so you'll know you've formatted your file properly if the indentation is 2 spaces.
| #include "TutorialServo.h" | ||
| #include "CANBus.h" | ||
| #include "CANMsg.h" | ||
| #include "mbed.h" |
There was a problem hiding this comment.
For your include statements:
- you dont need to include mbed.h (its already included in the header file)
- make sure you include hw_bridge.h
| } | ||
| } | ||
|
|
||
| TutorialServo servoObject; |
There was a problem hiding this comment.
be careful because these lines of code will never be run (your code will enter the while loop and never leave)
|
|
||
| int main() | ||
| { | ||
| CANBus can(CAN1_RX, CAN1_TX, HWBRIDGE::ROVERCONFIG::ROVER_CANBUS_FREQUENCY); |
There was a problem hiding this comment.
these declarations are well done (since they are right at the top of your main function), make sure to declare your servo object as one of the first things you do
No description provided.