tap_esc cleanup #9313
tap_esc cleanup #9313
Conversation
- removing unused variables - removing unused code blocks - improving code readability - initialising members at definition
src/drivers/tap_esc/tap_esc_common.h
Outdated
|
|
||
| int enable_flow_control(int uart_fd, bool enabled); | ||
|
|
||
| int send_packet(int uart_fd, EscPacket &packet, int responder); |
There was a problem hiding this comment.
Agreed, it would make more sense. However, send_packet will calculate and fill in the CRC checksum. That's why it cannot be const at this point. We could of course take the CRC calculation out of send_packet, but I'm not sure if that's more intuitive :)
There was a problem hiding this comment.
Ah I did not see that. It's fine then.
src/drivers/tap_esc/tap_esc.cpp
Outdated
|
|
||
| // clean up the alternate device node | ||
| //unregister_class_devname(PWM_OUTPUT_BASE_DEVICE_PATH, _class_instance); | ||
| if (tap_esc->init() != 0) { |
There was a problem hiding this comment.
There should be a nullptr check before dereferencing tap_esc
src/drivers/tap_esc/tap_esc.cpp
Outdated
| SCHED_PRIORITY_ACTUATOR_OUTPUTS, | ||
| 1100, | ||
| (px4_main_t)&run_trampoline, | ||
| nullptr); |
There was a problem hiding this comment.
If you pass argv here, you can do the argument parsing in instantiate and _channels_count, _device don't need to be static (the static variables will occupy the RAM even if the driver is not started).
src/drivers/tap_esc/tap_esc.cpp
Outdated
| @@ -84,19 +84,6 @@ | |||
| class TAP_ESC : public device::CDev, public ModuleBase<TAP_ESC>, public ModuleParams | |||
| { | |||
| public: | |||
| enum Mode { | |||
src/drivers/tap_esc/tap_esc.cpp
Outdated
| uint16_t rpm[TAP_ESC_MAX_MOTOR_NUM]; | ||
| memset(rpm, 0, sizeof(rpm)); | ||
| uint8_t motor_cnt = num_pwm; | ||
| static uint8_t which_to_respone = 0; | ||
| static uint8_t responder = 0; |
There was a problem hiding this comment.
Can you make this a class member instead of static?
tap_esc_common@bkueng Do you have time to review? It's low priority, and maybe you'll have to to through the individual commits in order to make sense of it.
The PR has been bench-tested on the Aero.