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 all 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.
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.
was this supposed to be [12] not [126] ?
we also need a count to be able to tell how many are valid, and count need to be uint16_t to get the right packing and zero truncation
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'd prefer to match the proposed DrroneCAN one, and make length 24, with index of the first cell filled
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.
@tridge
This is supposed to be 126. Thought was to reduce overheads of sending multiple messages and the hassle of re-assembling them until you get to 126 cells.
The array will be ordered last and any unused cells are zero-filled (so no cell count needed), so this will truncate on the number of cells in the battery. If we set this to 24 cells the only difference is that for more than 24 cells you'll have to reconstruct the full list of cells from two messages. What's the value in that other than being able to extend the message later?
Note - other than the length and the fact they implicitly have battery ID the UAVCAN message looks the same: https://github.com/dronecan/DSDL/pull/7/files
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 the purpose was that by mirroring both messages as close as possible there would be fewer implementation details?
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.
Fair enough. I'd like confirmation though, because that's not what it sounds like. I'm fairly confident of my understanding of MAVLink, but then this is Tridge, who is very rarely wrong.
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.
@tridge tridge Thanks, that's a typo. Fixed to 126.
But you also said
So I thought there might be an actual problem with the design of the message being 126. I can force this to 24 to match DroneCan if you feel strongly that is somehow better. The symmetry appeals, it's just I can't see a justification for it technically.
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 consider the is message "on hold" (though non blocking on the rest of it). From @tridge:
I've requested a PR to this message - ideally from @WickedShell
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.
see my proposal for a max/min cell voltage, much saner
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.
#19 (comment)
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'm happy with the addition min/max cell voltage to the main status message. Though you wouldn't know which cell is failing unfortunately.
The
BATTERY_CELL_VOLTAGES
message is still important though. But we should add a note to it such that systems default to NOT sending it.The complete record of cell voltages are rather useful for applications. Some of these could be done