Feat roboarm #23

Bob-Thomas · 2017-06-05T12:29:53Z

No description provided.

Stepper class to control the steps of the motor RobotArmController to controll 3 different steppers

…nterface

…t-roboarm-interface

…to arm interface

…tart position on startup

…t-roboarm-limitations

…t-roboarm-nonblocking

…t-roboarm

…limitations

Bob-Thomas · 2017-06-05T12:40:10Z

Bob Thomas

cjsmeele

The structure and overall quality of the code look good!
I found no major issues.

Most of my comments are about misformatted or missing docblocks (besides that, the documentation looked pretty neat).

Additionally there are some minor C++ issues.

Lastly, the hwlib PR for fixing hwlib::string: I hope you can get it merged soon, otherwise, to be able to merge a working version of this PR, we would have to change the submodule to a fork.

cjsmeele · 2017-06-05T15:40:51Z

modules/ROBOARM/src/ky101.cc

@@ -0,0 +1,7 @@
+#include "ky101.hh"


This file is missing a doxygen file header.
You can omit the \brief here, but at least mention the authors and copyright+license information.

cjsmeele · 2017-06-05T15:40:56Z

modules/ROBOARM/src/ky101.hh

+
+#include "wrap-hwlib.hh"
+
+//\brief Simple boundary class for the KY101 chip


This comment will not be picked up by doxygen.
Please use this style for doxygen documentation on classes:

/** * \brief Some short description */

cjsmeele · 2017-06-05T15:43:19Z

modules/ROBOARM/src/ky101.hh

+/**
+ * \file
+ * \brief     KY101 chip interface
+ * \author    Luke Rovers


Correct me if I'm wrong, but isn't your name spelled with two 'o'-s? :-)

Yeah XD that's my fault

cjsmeele · 2017-06-05T15:45:14Z

modules/ROBOARM/src/ky101.hh

+//\brief Simple boundary class for the KY101 chip
+class Ky101 {
+private:
+    // hwlib pin_in that the chip is wired to


For doxygen to pick up single-line brief comments, they should be formatted as such:

/// Some one-line description of what's below. int theThing;

cjsmeele · 2017-06-05T15:49:46Z

modules/ROBOARM/src/ky101.hh

+
+public:
+    // Constructor for the Ky101 classs
+    //\param signal hwlib::pin_in to read the input from


This is not a valid doxygen docblock.
For multiline docblocks, use the format as described in the coding guidlines at https://github.com/R2D2-2017/R2D2-2017/wiki/Coding-guidelines#param
The same goes for the function below.

cjsmeele · 2017-06-05T16:53:06Z

modules/ROBOARM/src/robot-arm.cc

+        case RobotAxis::X:
+            if ((checkLimitations() == RobotLimitSwitch::BOTH ||
+                 checkLimitations() == RobotLimitSwitch::X)   &&
+                !clockwise) {


Not a critical issue by any means, but consider checking the bool clockwise in the beginning of the condition. This way you can save two function calls involving I/O when clockwise is true.
The same goes for the Y axis check below.

Alternatively I suggest you call checkLimitations() just once in this function and save the results.

cjsmeele · 2017-06-05T16:58:20Z

modules/ROBOARM/src/robot-arm.cc

+#include "robot-arm.hh"
+
+RobotArmController::RobotArmController(Stepper &xAxis, Stepper &yAxis, Stepper &zAxis,
+                                       hwlib::target::pin_in &xLimitSwitch, hwlib::target::pin_in &yLimitSwitch,


Minor style question, up to you if you want to change it or not:
Putting the parameters on separate lines and aligning them would be incredibly aesthetically pleasing 😄 especially since they come in pairs.

cjsmeele · 2017-06-05T17:02:24Z

modules/ROBOARM/src/parser.cc

+Status parseCommand(const hwlib::string<0> &command, RobotArmController &robotArmController) {
+    uint8_t space = 0; // the place of the space
+
+    for (; command[space] != ' ' && space < command.length(); space++) // find space


In the condition, check space < command.length() first of the two to avoid a possible out of bounds read.

cjsmeele · 2017-06-05T17:04:57Z

modules/ROBOARM/src/parser.cc

+#include "parser.hh"
+
+Status parseCommand(const hwlib::string<0> &command, RobotArmController &robotArmController) {
+    uint8_t space = 0; // the place of the space


hwlib::string::length() returns size_t, match your type.

cjsmeele · 2017-06-05T17:06:49Z

modules/ROBOARM/src/parser.cc

+    bool    direction = 0; // true if we go counterclockwise (negative value)
+
+    // string to int routine because no stdlib and not present in hwlib :(
+    for (uint8_t i = 0; i < amount.length(); i++) {


Change i to size_t, matching the type of length().
In general, whenever you index or address something, use size_ts.

Bob-Thomas · 2017-06-05T20:42:59Z

@cjsmeele updated the pr

c q

… chip atm

Jipolie01

Looks good. Take care of Dave

Remco-Ruttenberg

Feature looks good

TheHippiez

Jip told me to do this

ManDeJan and others added 30 commits May 23, 2017 10:24

Added roboarm module

d75dd0b

Added initial parser structure

17459a8

Test with hwlib-string

629f87a

ROBOARM-01 robot arm controller draft

f26d722

Stepper class to control the steps of the motor RobotArmController to controll 3 different steppers

Merge remote-tracking branch 'origin/development' into feat-roboarm-i…

c893a48

…nterface

ROBOARM-01 code style fixes

19a6246

ROBOARM-01 more code style fixes

2d3a715

ROBOARM-01 code style fixes

5453bc1

ROBOARM-01 removed tripple slash comment

617a053

ROBOARM-01 final codestyle fixes?

57ea664

Updated parser

51e0a46

Merge pull request #12 from R2D2-2017/feat-roboarm-interface

c3bada9

Merge remote-tracking branch 'origin/development' into feat-roboarm

4ae3565

ROBOARM-01 calibrate axes

73965a8

[ROBOARM] Updated parser

05e83cc

[ROBOARM] Updated doxygen

7405220

ROBOARM-01 add all axes to control

65f48d5

Merge remote-tracking branch 'origin/development' into feat-roboarm-i…

658f15b

…nterface

[ROBOARM] Made use of hwlib features

c22abb1

Added limitation code in robot arm controller

4d06edf

Merge remote-tracking branch 'origin/feat-roboarm-interface' into fea…

d68cbb8

…t-roboarm-interface

[ROBOARM] more hwlib, more templates, more good

c7db145

more const, more good

dd76540

ROBOARM-01 Merged parser into arm interface and merged limitations in…

30bbd4f

…to arm interface

ROBOARM-01 Added KY101 functionality & implemented function to find s…

804724a

…tart position on startup

Merge remote-tracking branch 'origin/feat-roboarm-interface' into fea…

f794c93

…t-roboarm-limitations

Stepper motors now have basic non-blocking functionality

8a6ac6d

Merge remote-tracking branch 'origin/feat-roboarm-interface' into fea…

2dd8af7

…t-roboarm-nonblocking

Fixed motors not stopping at target

80eb1c3

Acceleration implementation try out

7c04842

Bob Thomas added 3 commits June 5, 2017 13:30

Merge remote-tracking branch 'origin/feat-roboarm-interface' into fea…

8cf1dca

…t-roboarm

ROBOARM-01 getting code up to style and making it compile

bef530f

Merge remote-tracking branch 'origin/feat-roboarm' into feat-roboarm-…

8f5f969

…limitations

Bob-Thomas requested review from Jipolie01, ManDeJan, Remco-Ruttenberg, TheHippiez, cjsmeele and sqrtroot June 5, 2017 12:29

ManDeJan added 3 commits June 5, 2017 15:19

[ROBOARM] Added reset functionality in parser and formattet main

a488db4

[ROBOARM] Formatting fixes

917d2cc

[ROBOARM] camelCaseVariabalsAreNowAThing

7998221

cjsmeele requested changes Jun 5, 2017

View reviewed changes

[ROBOARM-01] Lead review fixes (mostly doxygen styling)

a9c6eca

Bob Thomas and others added 11 commits June 5, 2017 23:23

[ROBOARM-01] tiny doxygen fix

c564c03

Added blocking test in main.cpp of src-folder

31b23db

Fixed merge conflicts

c5904f2

Added blockingtest to main.cc in src-folder

d42822b

c q

[ROBOARM-01] Hwlib update

5dbc4e9

[ROBOARM-01] Add new test and remove while loop cause we're missing a…

85a9bd0

… chip atm

[RABOARM-02] Applying RAMP shield

6949452

[ROBOARM-02] Command parser works

9dc5097

[ROBOTARM-02] added enable function

6d09153

[ROBOARM-02] testers added

b50e783

[ROBOARM-02] Docblocks updated

5191dfd

Jipolie01 approved these changes Jun 15, 2017

View reviewed changes

Remco-Ruttenberg approved these changes Jun 15, 2017

View reviewed changes

TheHippiez approved these changes Jun 15, 2017

View reviewed changes


		#include "wrap-hwlib.hh"

		//\brief Simple boundary class for the KY101 chip

Feat roboarm #23

Are you sure you want to change the base?

Feat roboarm #23

Uh oh!

Conversation

Bob-Thomas commented Jun 5, 2017

Uh oh!

Bob-Thomas commented Jun 5, 2017

Uh oh!

cjsmeele left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjsmeele Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bob-Thomas commented Jun 5, 2017

Uh oh!

Jipolie01 left a comment

Choose a reason for hiding this comment

Uh oh!

Remco-Ruttenberg left a comment

Choose a reason for hiding this comment

Uh oh!

TheHippiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

cjsmeele Jun 5, 2017 •

edited

Loading