vedderb / bldc

The VESC motor control firmware
2.14k stars 1.34k forks source link

Keep COMM_GET_VALUES packet length max 63 bytes #63

Closed Peemouse closed 5 years ago

Peemouse commented 6 years ago

Hi Benjamin,

Since FW3.40, COMM_GET_VALUES full packet became 64 bytes long. That breaks the compatibility with Arduino since the default buffer is 64 (SERIAL_RX_BUFFER_SIZE) and can store only 63 bytes (Ringbuffer).

A lot of projects use Arduino and without tweak it keeps them away from VESC-Project now.

Could we arrange to stay under the 63 bytes limit ? Maybe by adding a specific command (COMM_GET_VALUES_SHORT) ?

Thank you for you support.

Best regards,

Clément

vedderb commented 6 years ago

I can add a few shorter getter commands to the next version for different values, it probably makes sense for some other applications too.

Peemouse commented 6 years ago

I'm glad you agree with this issue. May I suggest to split values depending on their "utility" and refresh frequency needs :

COMM_GET_INSTANT_VALUES (that needs to be updated quite often) (about 27 bytes including command byte)

- mc_interface_temp_fet_filtered()
- mc_interface_temp_motor_filtered()
- mc_interface_read_reset_avg_motor_current()
- mc_interface_read_reset_avg_input_current()
- mc_interface_get_duty_cycle_now()
- mc_interface_get_rpm()
- GET_INPUT_VOLTAGE()
- mc_interface_get_fault();
- mc_interface_get_pid_pos_now()
- app_get_configuration()->controller_id

COMM_GET_INTEGRATED_VALUES (could be refreshed with a lower frequency, or even not needed in some applications such as driving motor via UART) (about 26 bytes including command byte)

- mc_interface_get_amp_hours(false)
- mc_interface_get_amp_hours_charged(false)
- mc_interface_get_watt_hours(false)
- mc_interface_get_watt_hours_charged(false)
- mc_interface_get_tachometer_value(false)
- mc_interface_get_tachometer_abs_value(false)
- app_get_configuration()->controller_id;

This is just a suggestion. :) What are your throughts ?

Let me know if you want me to apply changes and pull request to save you time.

Cheers,

Clément

Addy771 commented 6 years ago

I would suggest to leave existing messages as they were previously and add new messages if you need to make changes. Modifying the existing messages breaks compatibility with existing software.

For example, I wrote a data logger script in python for VESC. I used the pyvesc library to handle communication with the VESC. I had trouble getting this to work, because the COMM_GET_VALUES message had been changed since the last time pyvesc was updated.

I had to fix the pyvesc library to make my logger work. Other people using pyvesc would have to do the same as it does not seem to be actively maintained right now. Then, if COMM_GET_VALUES changes in the future, the library will be broken again and everyone will have to fix their scripts.

Peemouse commented 6 years ago

Sorry for the misunderstanding. I meant 2 new commands at the end of enum, without changing the existing COMM_GET_VALUE, of course ! That way, nothing's broken down.

Lzx-Fun commented 6 years ago

I hope to see the next version soon. Thank you, Benjamin. So I can see the data, and I hope that there will be no change in UART reading data in the future.

vedderb commented 6 years ago

The next update will have the new commands, but I have to finish a few things first.

I general I add new data to the end of the values, and new enums to the end of the list to keep compatibility. The only thing that changes significantly between versions are the mc and app configuration structures.

My plan for now is to add a COMM_GET_VALUES_SELECTIVE command, where 1-3 bytes with a bitmask have to be provided to choose which values to get. E.g. if you send

b[0] = COMM_GET_VALUES_SELECTIVE b[1] = 0b10110011 (0xB3) b[2] = 0b10000001 (0x81)

you will get

mc_interface_temp_fet_filtered() mc_interface_read_reset_avg_motor_current() mc_interface_read_reset_avg_input_current() mc_interface_get_duty_cycle_now() mc_interface_get_rpm() GET_INPUT_VOLTAGE() mc_interface_get_fault()

The number of bitmask bytes will be variable to make room for future additions.

That is the most simple and general solution I can think of. It should also be useful when polling data over the CAN-bus and you want to stay within 1 frame.

Peemouse commented 5 years ago

Hi Ben, Do you have an estimated date for release ? Thanks in advance. Clément

vedderb commented 5 years ago

The COMM_GET_VALUES_SELECTIVE command is now implemented, with the following details:

Additionally, there is now also a command named COMM_GET_VALUES_SETUP, also with an COMM_GET_VALUES_SETUP_SELECTIVE counterpart, that sends accumulated data back from all VESCs seen on the CAN-bus. This command can also send the speed back in m/s if the gear ratio, wheel diameter and motor pole count is configured (which are new options for the latest firmware). The accumulated CAN-bus data is useful if you want to get the total mah and wh count, as well as the total power usage of the setup.

It will probably take a week or two until I push the updates as I have been working on major improvements for the app, and support for profiles. The good news with this update is that other new commands are coming to enable profile support, namely COMM_SET_MCCONF_TEMP and COMM_SET_MCCONF_TEMP_SETUP, where a short configuration with only current, power, erpm (or m/s) and duty limits can be sent. The setup command allows using m/s and can also be applied to all VESCs on the CAN-bus if a flag is set. These commands also have a flag to specify whether they should be applied temporarily until the next reboot, or permanently. The temporary version can be applied without stopping the motor, which can be useful for some applications.

/Benjamin

Peemouse commented 5 years ago

Really nice features Ben ! That will help a lot, thanks. Clément

vedderb commented 5 years ago

The latest firmware with this implemented it now out, it would be great if you can give it a try. Took me a lot longer than I was hoping for, but it turned out to be trickier than I thought to find a way to communicate with all VESCs at the same time, use hardware-independent profiles and also take gearing, wheel size and battery values into account. When I implement something like this I want to get it mostly right from the start.

Ideally COMM_GET_VALUES_SELECTIVE and COMM_GET_VALUES_SETUP_SELECTIVE will keep working for resource-constrained hardware even when I add new data in later firmwares. The bitmask has plenty of space for new values in the future.

Regarding compatible communication, I will write some documentation about that soon. The thing that is most likely to keep changing between firmware releases (for good reasons) is the motor and app configuration, but the other commands should be quite backwards compatible. At one point, when moving from BLDC Tool to VESC Tool, I inserted a command in the middle, but in the future I will avoid that.

Peemouse commented 5 years ago

Thank you very much Benjamin for the explanation. I started modifying my Arduino library to take advantage of COMM_GET_VALUES_SELECTIVE. I saw that there are 32 selectable values (or groups of values like MOSFET temp). Should be fair enough for a while.

Thanks for having taken into consideration this issue. Backwards compatibility is also more than welcome.

Peemouse commented 5 years ago

Got it working very well. Thank again, it saves computation time on little projects requesting only a couple of datas.