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
RFC 0018 - Improved Battery Status Reporting #19
base: master
Are you sure you want to change the base?
RFC 0018 - Improved Battery Status Reporting #19
Changes from 44 commits
f72ae85
4a719f2
496d464
657d762
5c684c6
8247fbc
cdfcac1
28b23db
f110cfa
3e56f60
f15e542
05be55e
cc94393
9033cc9
42cfe84
dc63056
1a2fb83
8372532
57d9d9b
bfa89c4
660d9c3
2e63457
199a2c6
1295684
5e530eb
c8915b8
9acf267
45c8361
7f464ed
41a7fb8
506de46
af11eaf
9d17b55
abe7dce
469e5c3
ff245bb
e4e9b57
8b9625f
acc9e73
d563499
0f678c7
8801ef4
e5aeab5
f84e83b
5d19e49
75bdacc
5fb3ff8
ab3d25f
5fa4be2
6aa86db
ce99827
8e2deb7
b5f65c0
ea508a7
c25344b
12561b9
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.
Is there a need to support more than 255 batteries? @AlexKlimaj thoughts? Would you ever go bigger than 255 in your box?
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.
I hope not. That feels like excessive future proofing.
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.
I mean .. maybe ... is it? What if someone wanted to replace autopilot with ardupilot? 🤨
https://www.google.com/search?q=how+many+batteries+in+a+tesla
see what I did there I rhymed
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.
@dakejahl And you wonder why nobody likes you :-)
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.
haha ouch but you're wrong I am beloved. But no in DroneCAN I think it can make sense, probably not mavlink
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.
Yeah yeah :-). I'm going to say no "right now". Yes this might come back and bite us, but it feels like a waste.
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.
I've thought about that one before too.....
But i guess one could create their own message at that point. :)
For mavlink no reason to waste space.
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.
@dakejahl I might be overthinking this, but should this perhaps be "not ready to use" ? i.e it we're constructing it from all the other faults so we're not saying it is ready to use, but that it is "not unready to use".
As it is now, you'd have to set the bit even if you didn't know the readiness, and by setting it you're saying its good to go. Whereas if we reverse it we're just saying - some flag that indicates there is a problem is set.
Anyway, not certain on this. I like a readiness flag too - just not sure we can be ready = not unready
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.
@hendjoshsr71 First, thanks for the review. Looks like a lot more issues than I first thought
This is supposed to reflect "there are no known issues that would prevent this flying" so how about MAV_BATTERY_STATUS_FLAGS_NOT_READY_TO_USE ?
The actual issues that set this would depend on the person setting up the system. If it is OK to charge in place, then this would not be a fault that set this.
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.
I think you might 😆
Hmmm I'm not sure... You are saying you want it to be "not ready to use" since this would then correlate with a flag that gives us the reason for it being not ready? I would say you would infer this in the same way as seeing "ready to use" be set to zero. The other part is that an empty message (all zeroes) would be default to "not ready" which is a nice side effect.
Or maybe I don't see the point.
I don't understand how we can know it's "not ready" but can't know it's "ready"? They're the same thing just inverse.
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.
@dakejahl What we do is check a bunch of things which mean it is not ready to use. So what we are saying is there are no flags that indicate it is not ready to use. Then we're inverting that and saying, "if there are no flags that say it is not ready to use it is ready to use."
Are they semantically the same? Technically they are not, because we only detect "known issues". It could be that there is an unknown issue we do not know about - so we're telling people it is ready to use when it is not. A legal expert would care, but perhaps we do not need to.
It means that if you don't have the ability to report or detect faults you have to set this as ready all the time.
That's kind of my point - you're not actually saying its ready, you're just saying you don't know of any faults.
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.
Ahh so you're saying "the system is not not ready" does not mean the same thing as "the system is ready"...? Essentially saying we could just not know the state. I honestly don't have a strong opinion on it, I think semantically you are correct but I sure do hate double negatives lol. So yeah we can change it to "not ready" and then the check would be if it's unset, easy enough. I'll leave that up to you.