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

AP_BattMonitor: move read_block up to SMBus base class #20692

Merged
merged 1 commit into from Aug 29, 2022

Conversation

hendjoshsr71
Copy link
Member

This moves the read-block function to the base class.

It isn't a complete copy/paste since the three places each have their own implementations.

Tested with a SUI3 battery

image

@amilcarlucas
Copy link
Contributor

Nice catch, and saves memory:

Binary Name      Text [B]         Data [B]     BSS (B)      Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  ---------------  -----------  -----------  ----------------------------  -------------------------
ardusub          -296 (-0.0191%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0190%)                                  410876
arducopter       -296 (-0.0168%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0167%)                                  197484
blimp            -296 (-0.0229%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0229%)                                  671312
ardurover        -296 (-0.0186%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0186%)                                  372728
arducopter-heli  -296 (-0.0167%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0167%)                                  189916
arduplane        -296 (-0.0169%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0169%)                                  216536
antennatracker   -296 (-0.0219%)  0 (0.0000%)  0 (0.0000%)  -296 (-0.0219%)                                  612848

@WickedShell
Copy link
Contributor

WickedShell commented May 4, 2022

I've looked at this before, but I didn't have all the required battery types to do the required testing. If we can get the testing done, I'd be much happier with only one implementation of read block! The main ones that need to be tested to bring this in:

  • Solo
  • [x ] Hitatchi/Maxwell
  • [x ] Royote
  • SUI (marking as done since you had one to test obviously)

Bonus points if someone tests:

  • Tattue Smart Plus 2.0

@hendjoshsr71
Copy link
Member Author

So this is part of my split off as many small items from my much larger smbus PR as I can.

@amilcarlucas could you test the rotoye one? (It may be a bit before it I get mine from Rotoye.)

@rmackay9 I think you have a maxell battery could you test?

I have a Tattu Smart Battery Plus , without the 2.0 bit and a curious mini USB port???

I'm not sure who has a solo on the team?

@WickedShell
Copy link
Contributor

I have a Tattu Smart Battery Plus , without the 2.0 bit and a curious mini USB port???

I'm not sure about the original. I know some of the Smart Plus 2.0 had SMBus that I had tested as working with what used to be the maxell (now the generic) driver. The only thing that was off that I found was the temperature was C instead of kelvin, but that's a fairly low pain problem.

@rmackay9
Copy link
Contributor

@hendjoshsr71, my Maxell SMBus battery is completely dead I'm afraid (doesn't work with master either) but hopefully we can find someone else in Japan who can test.

@rmackay9
Copy link
Contributor

Should this be rebased on master? It looks to me like it's really old..

@hendjoshsr71
Copy link
Member Author

Should this be rebased on master? It looks to me like it's really old..

Rebased.

Fortunately, the only PRs of substance in this area have been my own.

@tatsuy
Copy link
Contributor

tatsuy commented Aug 29, 2022

@hendjoshsr71 I tested this with Maxell battery.
I could use it just as before.

@hendjoshsr71
Copy link
Member Author

@hendjoshsr71 I tested this with Maxell battery. I could use it just as before.

Thank you @tatsuy!

@tridge tridge merged commit a93b887 into ArduPilot:master Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants