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_ExternalAHRS: Change the define definition to an enum class #28104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

muramura
Copy link
Contributor

The define definition is grouped with a group name.
I will change it to ENUM CLASS.

@amilcarlucas
Copy link
Contributor

amilcarlucas commented Sep 14, 2024

Binary Name      Text [B]     Data [B]     BSS (B)      Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  -----------  -----------  -----------  ----------------------------  -------------------------
arducopter       0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      154952
antennatracker   0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      625228
arducopter-heli  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      155960
ardusub          0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      364492
ardurover        0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      306200
blimp            0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      603104
arduplane        0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      157896

@muramura Can you check if the binary output is equal as well?

@muramura
Copy link
Contributor Author

muramura commented Sep 14, 2024

@amilcarlucas san.

AFTER and BEFORE are the same size.

AFTER

BUILD SUMMARY
Build directory: /home/parallels/work2/ardupilot/build/fmuv3
Target Text (B) Data (B) BSS (B) Total Flash Used (B) Free Flash (B) External Flash Used (B)

bin/arducopter 1557289 3892 192868 1561181 519572 Not Applicable

BEFORE

BUILD SUMMARY
Build directory: /home/parallels/work2/ardupilot/build/fmuv3
Target Text (B) Data (B) BSS (B) Total Flash Used (B) Free Flash (B) External Flash Used (B)

bin/arducopter 1557289 3892 192868 1561181 519572 Not Applicable

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

I'm not in favour of this as it currently stands.

The static casts make it far less readable.

It could be done with something like a has_status method, but I don't think we need to go there.

I'm going to ping @valentinb on this PR - he's most recently changed this stuff and can either back me up or say he loves it and would love to have it pushed forward.

@@ -84,6 +84,54 @@ class AP_ExternalAHRS_InertialLabs : public AP_ExternalAHRS_backend {
INS_SOLUTION_STATUS = 0x54,
};

// unit status bits
enum class ILABS_UNIT_STATUS : uint16_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enum class ILABS_UNIT_STATUS : uint16_t {
enum class UnitStatus : uint16_t {

similarly elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the ENUM name.

GCS_SEND_TEXT(MAV_SEVERITY_CRITICAL, "ILAB: Accelerometers failure");
}

if (state2.unit_status & ILABS_UNIT_STATUS_MAG_FAIL) {
if (state2.unit_status & static_cast<uint16_t>(ILABS_UNIT_STATUS::MAG_FAIL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't tell me this looks better!

There are ways this could be done to look quite nice, but this is not one of them!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that by using ENUM CLASS, we can detect the bug in #28111 at the coding stage.

@muramura muramura force-pushed the AP_Change_the_define_definition_to_an_enum_class branch from c57b8e7 to e731615 Compare September 15, 2024 01:09
@muramura
Copy link
Contributor Author

#28111 fix has been included.

@valbv
Copy link

valbv commented Sep 16, 2024

Hello. A couple of days ago I updated the part with InertialLabs EAHRS handler
No need to mention "valentinb", he is another Valentine:)

Thanks for finding and fixing the bug

I can't say anything about using "define" or "enum class".
The abundance of "static_cast" really adds a lot of text.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Sep 16, 2024

An alternative approach to a static cast is to add an overload to the unary + operator on the enum class.
This is recommended by some developers on the C++ standard here:
https://groups.google.com/a/isocpp.org/g/std-proposals/c/eiN1Xqrt59Y/m/DscuM2ViDAAJ

Advantages:

  • Shorter syntax than static cast
  • Only allowed on enums you add the operator for
  • Preserves type safety of enum class

Disadvantages:

  • + as an operator isn't obvious to all devs what it does because it's a less common operator
  • You need to add this operator overload to every enum you want to cast in this way

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.

5 participants