Skip to content

libcreate driver minor changes/additions#29

Closed
betaupsx86 wants to merge 2 commits intoAutonomyLab:masterfrom
betaupsx86:master
Closed

libcreate driver minor changes/additions#29
betaupsx86 wants to merge 2 commits intoAutonomyLab:masterfrom
betaupsx86:master

Conversation

@betaupsx86
Copy link

Minor changes/additions to libcreate driver. Cliff Sensors, Wheeldrops

Added Cliff sensor signal readings. Cliff detection divided in Left, Front Left, Front Right, Right to give more info to the user. Right and Left Wheeldrop also divided for the sake of completion.
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR :)
Making the changes I suggested (leaving functions isCliff() and isWheeldrop()) should also fix the CI build.

}
}

bool Create::isCliff() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping this function is still useful. For instance, if the user is only interested in detecting a cliff for the purposes of stopping the robot (and does not care if it is left or right). But now the implementation can change:

return (isCliffLeft() || isCliffFrontLeft() || isCliffFrontRight() || isCliffRight());

}
}

bool Create::isWheeldrop() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the cliff API, I think this should be kept in addition to the new function calls:

return (isLeftWheeldrop() || isRightWheeldrop());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True I was thinking on terms of create_autonomy where we want to expose everything to the ROS environment and there is little need for isCliff or isWheeldrop. I shall add those functions back since libcreate can be used independently from ROS.

* \return true if a left or right wheeldrop is detected, false otherwise.
/* True if a left wheeldrop is detected.
*/
bool isLeftWheel() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renaming to isLeftWheeldrop() is more informative.

/* True if a right wheeldrop is detected.
*/
bool isWheeldrop() const;
bool isRightWheel() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simliarly: isRightWheeldrop()

return serial->send(cmd, 2);
}

bool Create::isLeftWheel() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is overindented.

}
}

uint16_t Create::getCliffSignalLeft() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the indentation of these functions to match the rest of the file.

}

uint16_t Create::getCliffSignalLeft() const {
if (data->isValidPacketID(ID_CLIFF_LEFT_SIGNAL)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this functionality? I don't expect ID_CLIFF_LEFT_SIGNAL to be evaluated as a valid packet since it is not added as a packet to request for any protocol in data.cpp:13. You can add them to that constructor for protocols V_2 | V_3, but beware the warning regarding flooding the serial. After you've made the changes you should be able to check if there are problems on the serial with calls to Create::getNumCorruptPackets().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I completely missed that. I assumed it was valid since it was defined on the headers. I'm going to try to add them to the protocols, but I won't have a Create 2 base to test them for at least a month. Maybe I should refrain from exposing the cliff signals until then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can test your changes as well when your ready.

@jacobperron
Copy link
Member

Most of the features were added in #48 and #49.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants