Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
✨ Add Support for the ADI Expander #245
✨ Add Support for the ADI Expander #245
Changes from 51 commits
1fd691c
d57786a
1fd125a
fd578fe
b8ef153
cdd6483
9457cc7
ae99033
bcf17fb
a461ede
5b9dae2
78bbc85
7e23948
6c89e26
65043e1
793157d
8dcc8d2
b0b1a6a
cc06e2e
fcfc966
877df53
320f2fe
853428c
c265df5
1f57bcb
1b0cb14
df7981b
bc9d9d2
4f59b3b
9fa3d95
5286d03
414ca7e
7df1ce8
919f4ce
2366756
6053a6b
3646819
0f68e42
8d80a18
12f0172
93ca9ff
aedc307
6487dc1
1e83096
82af00d
0c815d5
c65d583
e8bf9d6
4bf8135
b30c154
6e27dda
660e5cd
b4db9e2
f89fdf1
8d6ddbe
11668a1
f2e2439
997f116
2ad6900
a39bfe4
bc1695f
8267034
066e1df
5a38a5a
766ad6b
0ac5b5a
e980360
ac9d6b4
8442742
8119101
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is an override, then mark it as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn’t work because in order for the override command to compile, the original function has to be labeled as virtual. If you label the original as virtual, than gyro::get_value also tries to override it ev,en though it has a different return type. So, I don’t think there is a way of doing this, without sacrificing uniformity in function names, and it only adds slight maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm, this isn't really ideal. Since you have a virtual destructor, you already have the overhead of the vtable, so there is no good reason not to make the methods virtual. They should be virtual anyways, because if you have an
ADIUltrasonic
that is being treated as aADIPort
, you want it to properly dispatch to the derived method.However, I see the problem with clashing with
ADIGyro
. Unfortunately, how it is right now is not ideal. TheADIGyro
simply shadows and hides the derived method, which leads to problems with polymorphism and unexpected issues with the return type changing. I was worried it was going to lead to ambiguity, but at least this passes:But you can see how it is not ideal to have the return type change, it could lead to unexpected conversions and narrowing.
What would be ideal is to have proper overriding and matching return type. I'm not sure how that would be done though, so I might have to agree with your conclusion.
Maybe have the base
get_value
override return integers and have aget_value_HR
for the 10th precision, idk. If you and others don't think that's a good idea that's fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inheritence is private, so I don't think this is an issue.
BTW, my understanding is that most of the cost is in the function call. The vtable data structure has some size overhead, but that wouldn't be the part I worry about. If course, neither matter here.