bipropellant / bipropellant-protocol

defines and implements a low level protocol shared between a few different hoverboard repositories
MIT License
20 stars 17 forks source link

Improve documentation & consistency #38

Open Cirromulus opened 4 years ago

Cirromulus commented 4 years ago

Hi there! While using the protocol, some questions turned up. For example: The int32_t max_power parameter in protocol.h:50 is commented as // max speed in this mode, which seems like a copy-paste error. The parameter is also used in the c++ interface.

It seems as if max_power is the PWM-Value clamp (from main.c:551) ranging from 0-1000 which should actually be 1024 (10 bit PWM): CLAMP(pwms[i] + pwm, -SpeedData.speed_max_power, SpeedData.speed_max_power); This is somewhat conflicting with protocolfunctions.h:78:

PROTOCOL_SPEED_DATA SpeedData = {
    {0, 0},

    600, // max power (PWM)
    -600,  // min power
    40 // minimum mm/s which we can ask for
};

I am not asking for wiki or doxygen-style documentation. For me it would be enough if funtions or defines had small and consistent comments.

The same issue would be for bipropellant-hoverboard-api, but I did not want to duplicate this issue.

void HoverboardAPI::sendSpeedData(double left_speed, double right_speed, int16_t max_power, int16_t min_speed, char som)

Thanks

123JRM commented 3 years ago

Agree.

btsimonh commented 3 years ago

there was a lot of learning going on at the time. I think we found that over 600 started to 'break' things.... i.e. if you took the PWM past this point, at a hardware level things started to break down (involved look at ST docs required!).