Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Protocol refactor #46

Open
wants to merge 7 commits into
base: cart-pole-3
Choose a base branch
from
Open

Conversation

AndBondStyle
Copy link
Member

No description provided.

@AndBondStyle AndBondStyle changed the base branch from master to cart-pole-3 July 3, 2023 03:18
@dasimagin
Copy link
Contributor

Перед мерджем не забудьте пересобрать контейнер.

@dasimagin
Copy link
Contributor

dasimagin commented Nov 26, 2023

  • cartpole/device/* – решил просто вам довериться, не стал смотреть ;)
  • scripts/compile_protobuf.py – не стал смотреть, доверяю вам
  • cartpole/log.py – тоже просто глаза вытекают... смесь смены стайлгайда миллиона комментов и чего-то еще :(

betterproto = {version = "^2.0.0b6", allow-prereleases = true}
crc = "^4.3.0"
cobs = "^1.2.0"
jsonref = "^1.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем?

@@ -40,6 +36,10 @@ torch = [
{url="https://download.pytorch.org/whl/cpu/torch-2.0.1%2Bcpu-cp310-cp310-win_amd64.whl", markers="platform_system == 'Windows' and platform_machine == 'amd64'"}
]
varint = "^1.0.2"
betterproto = {version = "^2.0.0b6", allow-prereleases = true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Не боимся юзать бету?

Target target = 5;
}
enum HardwareError {
NO_ERRORS = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Мне почему-то хочется в единственном числе, но дело вкуса...

optional float pole_angle = 4; // [rad] Current pole angle
optional float pole_angular_velocity = 5; // [rad/s] Current pole angular velocity
optional int32 error = 6; // Global error code
optional int32 hardware_errors = 7; // Hardware error bitmask
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. А почему все они optional? Вроде бы в состоянии нет опциональных полей.
  2. МБ тогдак и назвать hardware_error_bitmask или hardware_error_flags?

P.S. Надеюсь, 32 бита хватит на hardware_errors :)

RESET = 5;
RESET = 0;
CONFIG = 1;
TARGET = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Отдельно взять состояние потеряли возможность? Или это делается через пустой TARGET?

static constexpr float HOMING_SPEED = 0.4;
static constexpr float HOMING_ACCELERATION = 1.0;

static const int METERS_TO_STEPS_MULTIPLIER = MICROSTEPS * FULL_STEPS_PER_METER;
Copy link
Contributor

Choose a reason for hiding this comment

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

А почему это не float? Мы не теряем тут точности, неужели там такое кратное попадание?

} else if (std::abs(state.cart_acceleration) > config.max_cart_acceleration) {
state.error = Error_CART_ACCELERATION_OVERFLOW;
}
if (state.error) stepper.disable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Так грязновато делать, это словно неявное действие, которое название не отражает :)

Comment on lines +92 to +95
protocol.resetCallback = reset;
protocol.targetCallback = setTarget;
protocol.configCallback = setConfig;
stepper.homingCallback = homingCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

А в чем смысл все колбэки динамически тут выставлять, а не во время компиляции?


void homingCallback() {
if (!stepper.getErrors()) {
config.max_cart_position = stepper.getFullRange() / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Сбиваем то, что задал нам пользователь... сильно :)

Comment on lines +41 to +45
if (target.has_position) {
stepper.setMaxAccel(accel);
stepper.setMaxSpeed(velocity);
stepper.setPosition(target.position);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

У вас многие штуки построенные на асинхронных тасках, тогда надо заботиться об атомарности всех гетеров/сеттеров и вообще делать все с идеей, что мир асинхронен.

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

Successfully merging this pull request may close these issues.

2 participants