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

AP_ESC extended status packet addition #27248

Conversation

Pradeep-Carbonix
Copy link
Contributor

Added support for ESC extended status telemetry data for CubeOrange and AP_Periph :

  • INPUT_DUTY,
  • OUTPUT_DUTY,
  • STATUS_FLAGS

@andyp1per
Copy link
Collaborator

Do you have a spec for this? I am not finding anything that specifies this.

@amilcarlucas
Copy link
Contributor

Binary Name      Text [B]         Data [B]     BSS (B)        Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  ---------------  -----------  -------------  ----------------------------  -------------------------
ardusub          1320 (+0.0838%)  0 (0.0000%)  0 (0.0000%)    1320 (+0.0837%)                                  387144
antennatracker   1252 (+0.0944%)  0 (0.0000%)  -4 (-0.0015%)  1252 (+0.0942%)                                  636060
ardurover        1244 (+0.0760%)  0 (0.0000%)  -4 (-0.0015%)  1244 (+0.0759%)                                  326044
arduplane        1376 (+0.0773%)  0 (0.0000%)  0 (0.0000%)    1376 (+0.0771%)                                  179576
arducopter-heli  1248 (+0.0697%)  0 (0.0000%)  0 (0.0000%)    1248 (+0.0696%)                                  171368
blimp            1268 (+0.0939%)  0 (0.0000%)  -4 (-0.0015%)  1268 (+0.0938%)                                  612848
arducopter       1268 (+0.0711%)  0 (0.0000%)  -4 (-0.0015%)  1268 (+0.0709%)                                  176768

@loki077
Copy link
Contributor

loki077 commented Jun 11, 2024

@andyp1per it is a custom APD code which is made for us. There is no document for it.
Even if ardupilot don't accept the APDF120 custom implementation we would still like the ESCX backend support for logging to be merged.

@loki077
Copy link
Contributor

loki077 commented Jun 11, 2024

This is the telemetry Structure :
The flags:
// Enumeration defining bit positions for each flag
typedef enum
{
overTemp_flag = 0,
signalLoss_flag, // 200mS since last throttle packet
overVoltage_flag,
sysRebooted_flag, // After every reboot or power cycle this bit will stay high for 30 seconds for cube to capture it.
} flagTypes;

The telem struct:
typedef struct
{
int8_t temperature;
uint16_t voltage;
uint16_t current;
uint16_t consumption;
uint16_t rpm;
uint8_t inputDuty;
uint8_t outputDuty; //mS
uint8_t flags;
uint8_t crc;
}attribute((packed)) telem_t;

@loki077
Copy link
Contributor

loki077 commented Jun 11, 2024

@Pradeep-Carbonix @robertlong13 would be a good idea to split this into 2 PR one for ESCX support and other for APDF120 custom Firmware.

libraries/AP_BLHeli/AP_BLHeli.cpp Outdated Show resolved Hide resolved
@andyp1per
Copy link
Collaborator

Even if ardupilot don't accept the APDF120 custom implementation we would still like the ESCX backend support for logging to be merged.

So its not BLHeli at all. I think we should make this clear in the naming and in the define to enable it. EDT2 is also providing some of this information and some is also available in the BLHeli status frame - we need to not get these muddled up and not trample over one.

Copy link
Collaborator

@andyp1per andyp1per left a comment

Choose a reason for hiding this comment

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

Like EDTv2 I think we need to be able to compile this out with something like:
AP_EXTENDED_APD_TELEM_ENABLE

libraries/AP_BLHeli/AP_BLHeli.cpp Outdated Show resolved Hide resolved
libraries/AP_ESC_Telem/AP_ESC_Telem.cpp Show resolved Hide resolved
@Pradeep-Carbonix Pradeep-Carbonix force-pushed the APD_F120_new_packet_modification branch from 9da04c7 to bde0161 Compare June 25, 2024 06:23
@Pradeep-Carbonix
Copy link
Contributor Author

This PR now has changes related to DroneCAN ESCX implementation only. BLHeli implementation is removed from this PR.

Copy link
Collaborator

@andyp1per andyp1per left a comment

Choose a reason for hiding this comment

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

Need a different name but changes looking good.

libraries/AP_HAL/AP_HAL_Boards.h Outdated Show resolved Hide resolved
@Pradeep-Carbonix Pradeep-Carbonix changed the title Apd f120 new packet modification AP_ESC extended status packet addition Jun 26, 2024
@Pradeep-Carbonix Pradeep-Carbonix force-pushed the APD_F120_new_packet_modification branch from bde0161 to 8195e84 Compare June 28, 2024 00:31
libraries/AP_HAL/AP_HAL_Boards.h Outdated Show resolved Hide resolved
libraries/AP_ESC_Telem/LogStructure.h Outdated Show resolved Hide resolved
libraries/AP_ESC_Telem/AP_ESC_Telem.cpp Outdated Show resolved Hide resolved
libraries/AP_ESC_Telem/AP_ESC_Telem.cpp Outdated Show resolved Hide resolved
@Pradeep-Carbonix Pradeep-Carbonix force-pushed the APD_F120_new_packet_modification branch from 966454b to 9c9e00b Compare July 11, 2024 06:06
@Pradeep-Carbonix
Copy link
Contributor Author

Pradeep-Carbonix commented Jul 12, 2024

CI check autotest (sitltest-plane) is failing.
Tried to rerun on our VM -
./Tools/autotest/autotest.py --no-clean build.Plane test.Plane.SDCardWPTest
PASSED STEP: test.Plane.SDCardWPTest at Fri Jul 12 11:39:05 2024

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

this is getting close now, thanks!

Tools/scripts/build_options.py Outdated Show resolved Hide resolved
libraries/AP_DroneCAN/AP_DroneCAN.h Show resolved Hide resolved
@tridge tridge removed the DevCallEU label Jul 17, 2024
@@ -188,6 +188,10 @@
#define AP_EXTENDED_DSHOT_TELEM_V2_ENABLED HAL_REQUIRES_BDSHOT_SUPPORT
#endif

#ifndef AP_EXTENDED_ESC_TELEM_ENABLED
#define AP_EXTENDED_ESC_TELEM_ENABLED (CONFIG_HAL_BOARD == HAL_BOARD_SITL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you not want it enabled on more than sitl? Incidentally, since this is really just to handle dronecan I think the define should be based on drone can being available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling this feature for sitl up in the previous discussion. Mainly to help with autotests.
AP_ESC_Telem_config.h is added with -
#ifndef AP_EXTENDED_ESC_TELEM_ENABLED
#define AP_EXTENDED_ESC_TELEM_ENABLED HAL_ENABLE_DRONECAN_DRIVERS
#endif

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just enable for all drone CAN builds.

@Pradeep-Carbonix Pradeep-Carbonix force-pushed the APD_F120_new_packet_modification branch from 9c9e00b to cdfb37d Compare July 23, 2024 01:31
Comment on lines +70 to +79
#if AP_EXTENDED_ESC_TELEM_ENABLED
// get an individual ESC's input duty if available, returns true on success
bool get_input_duty(uint8_t esc_index, uint8_t& input_duty) const;

// get an individual ESC's output duty if available, returns true on success
bool get_output_duty(uint8_t esc_index, uint8_t& output_duty) const;

// get an individual ESC's status flags if available, returns true on success
bool get_flags(uint8_t esc_index, uint32_t& flags) const;
#endif // AP_EXTENDED_ESC_TELEM_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The APD ESC extended status telemetry messages implementation utilizes these functions. Given its specificity to our requirements, we decided to create two separate pull requests.

The first pull request includes the driver implementation (which is this one), as we believe it could be beneficial to a broader audience.
The second pull request will contain the AP_Periph interface with the APD ESC, which we plan to submit at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

If they are unused they should not be included because they cannot have been tested. You should add them in the future PR with the periph interface.

@@ -62,6 +62,8 @@ def __init__(self, filename, nm="arm-none-eabi-nm", strings="strings"):
('HAL_EFI_ENABLED', 'AP_EFI::AP_EFI',),
('AP_EFI_{type}_ENABLED', 'AP_EFI_(?P<type>.*)::update',),

('AP_EXTENDED_ESC_TELEM_ENABLED', 'AP_Extended_Telem::get_input_duty',),
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this won't work because the get_input_duty function is not used anywhere so the linker will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The APD ESC extended status telemetry messages implementation utilizes these functions. Given its specificity to our requirements, we decided to create two separate pull requests.

The first pull request includes the driver implementation (which is this one), as we believe it could be beneficial to a broader audience.
The second pull request will contain the AP_Periph interface with the APD ESC, which we plan to submit at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

AP_Extended_Telem::get_input_duty is not even a function that exists in this PR. As I say, even if it had the correct class name it would not work for extract features because its not used so it won't be in the compiled output which extract features uses.

*/
void AP_DroneCAN::handle_esc_ext_status(const CanardRxTransfer& transfer, const uavcan_equipment_esc_StatusExtended& msg)
{
#if HAL_WITH_ESC_TELEM
Copy link
Member

Choose a reason for hiding this comment

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

We should add a error somewhere if you get AP_EXTENDED_ESC_TELEM_ENABLED and not HAL_WITH_ESC_TELEM

Copy link
Member

Choose a reason for hiding this comment

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

And then we won't need this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @IamPete1, I have added the lines suggested into AP_ESC_Telem_config.h

@IamPete1
Copy link
Member

@Pradeep-Carbonix I have done some the fixes for this PR here: https://github.com/IamPete1/ardupilot/tree/extended_esc. I can push over this branch if that is OK with you. Or you could do it yourself.

- DroneCAN Callback function - handle_esc_ext_status definition
- ESCX structure definition
- Backend function to update Extended telemetry data (ESCX)
- Logging ESCX data
- ESCX conditional compilation directives
@IamPete1
Copy link
Member

Just to re-iterate, I have done all the fixes that I think this PR needs here: https://github.com/IamPete1/ardupilot/commits/extended_esc/

@tridge
Copy link
Contributor

tridge commented Aug 6, 2024

patches merged by Pete's PR #27755

@tridge tridge closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants