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.

Sign up for GitHub

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

Jump to bottom

Adding a v0.2 for the uavcan battery (BMS) parameters and status #105

Merged
merged 13 commits into from
Feb 3, 2021
Merged

Adding a v0.2 for the uavcan battery (BMS) parameters and status #105

merged 13 commits into from
Feb 3, 2021

Conversation

echoGee
Copy link
Contributor

@echoGee echoGee commented Jan 28, 2021

Main changes:

  • Adding individual cell voltages
  • More comments on the parameters

kyle-pawlowski and others added 2 commits January 27, 2021 17:31
@kyle-pawlowski
Update reg.drone.service.battery.Status.0.1 to include an array of ce…
67c238a 
…ll voltages and update reg.drone.service.battery.Parameters.0.1 to remove cell count now provided by cell voltages. Status.0.1 and Parameters.0.1 are now deprecated.
Adding comments for few of the parameters in parameters message for c…
48da83b 
…larity.
@echoGee echoGee mentioned this pull request Jan 28, 2021
Closed
@pavel-kirienko pavel-kirienko self-assigned this Jan 28, 2021
@pavel-kirienko pavel-kirienko linked an issue Jan 28, 2021 that may be closed by this pull request
[DS-015] Suggestion to include all the individual cell voltages of the pack on the bms status #102
Closed
pavel-kirienko
pavel-kirienko requested changes Jan 28, 2021
View reviewed changes
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

We can also include clarifications from #104 here if necessary.

reg/drone/service/battery/Parameters.0.2.uavcan Outdated Show resolved Hide resolved
reg/drone/service/battery/Parameters.0.2.uavcan Outdated Show resolved Hide resolved
reg/drone/service/battery/Parameters.0.1.uavcan Outdated Show resolved Hide resolved
reg/drone/service/battery/Status.0.1.uavcan Outdated Show resolved Hide resolved
reg/drone/service/battery/Status.0.2.uavcan Outdated Show resolved Hide resolved
reg/drone/service/battery/Parameters.0.2.uavcan Outdated Show resolved Hide resolved
@pavel-kirienko pavel-kirienko mentioned this pull request Jan 28, 2021
Closed
echoGee and others added 4 commits January 28, 2021 21:55
@echoGee @pavel-kirienko
Update reg/drone/service/battery/Status.0.2.uavcan
6932b5d 
Co-authored-by: Pavel Kirienko <[email protected]>
@echoGee @pavel-kirienko
Update reg/drone/service/battery/Parameters.0.2.uavcan
53fc334 
Co-authored-by: Pavel Kirienko <[email protected]>
Cleaning up "deprecated"
29ad687
1. spell error correction
3556991 
2. Detailed and generic explanation of the different terms used in this message. The terms apply to most rechargeable batteries. Although the some may be irrelevant
echoGee
echoGee commented Jan 29, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

Fixing some CI issues and others based on Pavel comment

echoGee
echoGee commented Jan 29, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

Adding comment in reference to #104 for changing vendor id to represent 32 bits instead of the original 40 bits

echoGee
echoGee commented Jan 29, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

Clarifying what charge_voltage is

echoGee
echoGee commented Feb 1, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

Fixing grammatical weirdness

@echoGee
Copy link
Contributor Author

echoGee commented Feb 1, 2021

Added all requested changes including #104

@pavel-kirienko pavel-kirienko self-requested a review February 1, 2021 17:18
pavel-kirienko
pavel-kirienko requested changes Feb 1, 2021
View reviewed changes
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Thanks. Please fix the CI and re-request review when ready.

echoGee
echoGee commented Feb 1, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

trailing space

@echoGee
Copy link
Contributor Author

echoGee commented Feb 2, 2021

/home/travis/build/UAVCAN/public_regulated_data_types/reg/drone/service/battery/Parameters.0.2.uavcan:68: {54} /home/travis/build/UAVCAN/public_regulated_data_types/reg/drone/service/battery/Parameters.0.1.uavcan:54: {54} /home/travis/build/UAVCAN/public_regulated_data_types/reg/drone/service/battery/Status.0.2.uavcan:38: {512, 514, 516, 518, 520, 522, 524, 526, 528, 530, 532, 534, 24, 26, 28, 30, 32, 34, 36, 38, 40, 42, 44, 46, 48, 50, 52, 54, 56, 58, 60, 62, 64, 66, 68, 70, 72, 74, 76, 78, 80, 82, 84, 86, 88, 90, 92, 94, 96, 98, 100, 102, 104, 106, 108, 110, 112, 114, 116, 118, 120, 122, 124, 126, 128, 130, 132, 134, 136, 138, 140, 142, 144, 146, 148, 150, 152, 154, 156, 158, 160, 162, 164, 166, 168, 170, 172, 174, 176, 178, 180, 182, 184, 186, 188, 190, 192, 194, 196, 198, 200, 202, 204, 206, 208, 210, 212, 214, 216, 218, 220, 222, 224, 226, 228, 230, 232, 234, 236, 238, 240, 242, 244, 246, 248, 250, 252, 254, 256, 258, 260, 262, 264, 266, 268, 270, 272, 274, 276, 278, 280, 282, 284, 286, 288, 290, 292, 294, 296, 298, 300, 302, 304, 306, 308, 310, 312, 314, 316, 318, 320, 322, 324, 326, 328, 330, 332, 334, 336, 338, 340, 342, 344, 346, 348, 350, 352, 354, 356, 358, 360, 362, 364, 366, 368, 370, 372, 374, 376, 378, 380, 382, 384, 386, 388, 390, 392, 394, 396, 398, 400, 402, 404, 406, 408, 410, 412, 414, 416, 418, 420, 422, 424, 426, 428, 430, 432, 434, 436, 438, 440, 442, 444, 446, 448, 450, 452, 454, 456, 458, 460, 462, 464, 466, 468, 470, 472, 474, 476, 478, 480, 482, 484, 486, 488, 490, 492, 494, 496, 498, 500, 502, 504, 506, 508, 510} /home/travis/build/UAVCAN/public_regulated_data_types/reg/drone/service/battery/Status.0.1.uavcan:39: {23} /home/travis/build/UAVCAN/public_regulated_data_types/reg/drone/service/battery/Status.0.2.uavcan:35: Trailing line comments shall be separated from the preceding text with at least two spaces

Is this a bug on the CI? The line 35, 39 and ones with a comment in the line after the parameter.

@pavel-kirienko
Copy link
Member

It is not a bug. It is saying that you need at least two spaces left of # and at least one on the right. Currently you have one on the left and zero on the right.

echoGee
echoGee commented Feb 2, 2021
View reviewed changes
Copy link
Contributor Author

@echoGee echoGee left a comment

Choose a reason for hiding this comment

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

CI trailing space fix

@echoGee
Copy link
Contributor Author

echoGee commented Feb 3, 2021

Anything pending on this ?

@pavel-kirienko
Copy link
Member

Didn't realize it's ready for review, will have a look soon. Next time please request a review explicitly to avoid delays:

image

@pavel-kirienko pavel-kirienko merged commit 7f5489e into OpenCyphal:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clyde-johnston clyde-johnston clyde-johnston left review comments

@pavel-kirienko pavel-kirienko pavel-kirienko approved these changes

@dimracer dimracer dimracer approved these changes

Assignees

@dimracer dimracer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DS-015] Suggestion to include all the individual cell voltages of the pack on the bms status
5 participants
@echoGee @pavel-kirienko @dimracer @clyde-johnston @kyle-pawlowski