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

save some log structure flash when features disabled #28788

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Dec 1, 2024

this continues some work that @peterbarker has started. I ran across one of these while working on a different PR

@tridge tridge requested a review from peterbarker December 1, 2024 04:51
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.

../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp: In member function 'void AP_OABendyRuler::Write_OABendyRuler(uint8_t, bool, float, float, bool, float, const Location&, const Location&) const':
../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp:23:46: error: 'ahrs' is not a member of 'AP'
   23 |         yaw         : (uint16_t)wrap_360(AP::ahrs().yaw_sensor * 0.01f),
      |                                              ^~~~

... that was the first error which popped up; consider using ./Tools/autotest/test_build_options.py with --define-match-glob to see if it picks up the others.

@@ -106,6 +107,9 @@ struct PACKED log_OD_Visgraph {
int32_t Lon;
};

#if !AP_AVOIDANCE_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to put the positive case first - ie. don't negate this and put the empty case at the end.

Not all that fussed, mind you.

@tridge
Copy link
Contributor Author

tridge commented Dec 1, 2024

... that was the first error which popped up; consider using ./Tools/autotest/test_build_options.py with --define-match-glob to see if it picks up the others.

ok, thanks. Started a run with "--define-match-glob=AP*" - does that sound right?

@tridge
Copy link
Contributor Author

tridge commented Dec 1, 2024

../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp: In member function 'void AP_OABendyRuler::Write_OABendyRuler(uint8_t, bool, float, float, bool, float, const Location&, const Location&) const':
../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp:23:46: error: 'ahrs' is not a member of 'AP'
23 | yaw : (uint16_t)wrap_360(AP::ahrs().yaw_sensor * 0.01f),
| ^~~~

that error also happens with master, it doesn't have anything to do with this PR
ie. this configure:

./waf configure --disable-AC_AVOID --disable-MODE_FOLLOW

produces:

../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp: In member function ‘void AP_OABendyRuler::Write_OABendyRuler(uint
8_t, bool, float, float, bool, float, const Location&, const Location&) const’:
../../libraries/AC_Avoidance/AC_Avoidance_Logging.cpp:23:46: error: ‘ahrs’ is not a member of ‘AP’
   23 |         yaw         : (uint16_t)wrap_360(AP::ahrs().yaw_sensor * 0.01f),
      |                                              ^~~~

@tridge tridge force-pushed the pr-log-structure-reduce branch from 644287f to 198e975 Compare December 17, 2024 00:34
@peterbarker
Copy link
Contributor

... that was the first error which popped up; consider using ./Tools/autotest/test_build_options.py with --define-match-glob to see if it picks up the others.

ok, thanks. Started a run with "--define-match-glob=AP*" - does that sound right?

Nope, --define-match-glob=AP_AVOIDANCE_ENABLED as that's the define in play in this PR

@peterbarker peterbarker force-pushed the pr-log-structure-reduce branch 3 times, most recently from a0ab0a6 to e1366c1 Compare January 4, 2025 07:45
@peterbarker peterbarker force-pushed the pr-log-structure-reduce branch from 5aecb81 to 99e7f51 Compare January 8, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants