search

RoboticsBrno / RB-ev3rt-hrp2-sdk

Apache License 2.0
0 stars 0 forks source link

Code review: ev3cxx_motor and ev3cxx_display API #1

Open JarekParal opened 7 years ago

JarekParal commented 7 years ago

Could you please do code review in last commit ev3cxx (https://github.com/RoboticsBrno/RB-ev3rt-hrp2-sdk/commit/49d3506e8168fe887b73b67ce79556a90b88c040) @yaqwsx @cednik?

yaqwsx commented 7 years ago

ev3cxx_motor.h:28 It is more natural to write "Port number" instead of "Number of port" ev3cxx_motor.h:43 Break vs brake (it occurs on multiple places) ev3cxx_motor.h I don't like names 'onForDegrees' and 'onForRotations' - why not rotateDegrees? Similarly, unregulated seems weird. ev3cxx_motor.h From docs it is not clear whether 'unregulated' means PWM period or actual speed (which is regulated). If it is a speed - what are the units?

ev3cxx_motor.cpp:14 Why not to throw an exception, if motor configuration is invalid? ev3cxx_motor.cpp:22 There should be brackets to prevent confusion with operator = ev3cxx_motor.cpp:28 Extra space ev3cxx_motor.cpp:26 Error handling is not good. In some cases, error from set_power is handled, in other case from motor_config. ev3cxx_motor.cpp:32 Is it ok, that if no error occurred a I reset the motor power? ev3cxx_motor.cpp:35, 49 assert(true) does nothing... ev3cxx_motor.cpp:59 Extra space

I also miss meaning of ER return type. It is not documented. Also it is not clear, why functions like off or on could fail, but onForDegrees can't fail.

JarekParal commented 7 years ago

ev3cxx_motor.h:28 Yes - "Number of port" => "Port number" ev3cxx_motor.h:43 Break => brake (found just once) ev3cxx_motor.h onForDegrees' and 'onForRotations' - I would like to have similar API as original LEGO software => It could be better for beginners. ev3cxx_motor.h docs 'unregulated' => repaired, speed - units => probably without exact units [TODO: check]

ev3cxx_motor.cpp:14 Begginner probably couldn't catch exception and errors in EV3RT are weird. Just check if is type or port in required range (motor_port_t and motor_type_t) but without cast (static_cast) you can't set that wrong. EV3RT doesn't check the real state of components. I probably throw out error checking. ev3cxx_motor.cpp:22 => Fix ev3cxx_motor.cpp:28 Extra space => Fix ev3cxx_motor.cpp:26 Firstly I must solve using error. ev3cxx_motor.cpp:32 It doesn't reset, it sets motor power if no error. ev3cxx_motor.cpp:35, 49 assert(true) - Fix => assert(false) ev3cxx_motor.cpp:59 Extra space => Fix

I must solve the question about checking EV3RT error. It is probably not useful in CPP API.