Pull Request SoC with Extended Kalman Filter #130

mulles · 2022-04-06T14:45:01Z

-Debugging is still done partially by printf, as I was not able to get the zephyr tool for it running.
-The equation system for the model is probably incorrect, we should discuss it.
-Licensing for TinyEKF is still unclear, we might need to -"include a copy of the MIT License in the root directory of the package." -> https://github.com/simondlevy/TinyEKF/blob/master/LICENSE.md These is a little wreid, as it promotes the idea that the whole charge-controller-firmware is under MIT License
-Similar problems exists for the code used from kalman-soc repo: the https://github.com/okrasolar/kalman-soc, which is as well under MIT License.

-Debugging is still done partially by printf, as I was not able to get the zephyr tool for it running. -The equation system for the model is probably incorrect, we should discuss it.

martinjaeger

Thanks a lot @mulles for the PR and the effort that went into it! And sorry for my delayed review.

I've added a few first comments, most of which are minor style related improvements.

In terms of functionality: As far as I understood, this would currently only work for 12V lead-acid batteries, correct? I think in that case we can't merge it to the main branch at the moment, as the charge controller can be used with a large variety of battery types.

We should either be able to switch back to the previous (simple) SOC algorithm or provide parameters like OCV curves etc. for other types as well.

In order to go ahead with the development for your thesis we could also start merging it to a separate branch (e.g. kalman-soc) and only when all batteries are supported and we have validated the algorithm a bit more we merge it to the main branch. What do you think?

martinjaeger · 2022-05-04T16:59:12Z

app/src/bat_charger.cpp

+    // TODO will need to add 24 V compatability
+    const uint32_t soc_scaled_hundred_percent = 100000;
+    const uint8_t voltages_size = 10;
+    const float batt_soc_voltages[voltages_size] = { 12720, 12600, 12480, 12360, 12240,
+                                                     12120, 12000, 11880, 11760, 11640 };


We're not only supporting 24V batteries in the charge controller, but also other cell chemistries than lead-acid batteries, including Li-ion NMC and LFP. Probably the OCV would have to be stored on a per-cell-basis (as the other parameters in the struct BatConf).

Correct, for quick convergence, it is good to get initial SoC close, so it is preferable to use different OCV-SoC curves for different chemistries. We might just multiply this curves with 2 to enable 24V-systems.

martinjaeger · 2022-05-04T17:02:36Z

app/src/bat_charger.cpp

+            break;
+        }
+    }
+    return (voltages_size - index) * (soc_scaled_hundred_percent / voltages_size);


We should better use linear interpolation and not use the nearest value. There is a function in the helper.cpp of the BMS firmware which could be used.

I guess, it is not important, as the initial SoC will be corrected by Kalman filter pretty fast.
Thus, it would not be my priority to implement this. You are free to improve it. The possible increase in CPU-cycles should be negligible.

app/src/main.cpp

martinjaeger · 2022-05-04T17:08:02Z

app/src/bat_charger.h

+    /**
+     * SOC estimation
+     *
+     * WARNING: TODO obsolte function, replaced by void charge_control(BatConf *bat_conf);


Isn't it replace by the new update_soc function with the additional EKF parameter?

martinjaeger · 2022-05-04T17:09:46Z

app/src/bat_charger.h

+    float tmp4[NUMBER_OF_OBSERVABLES_SOC][NUMBER_OF_OBSERVABLES_SOC];
+    float tmp5[NUMBER_OF_OBSERVABLES_SOC];
+
+} EkfSoc;


Can't we put this into kalman_soc.h? It woul separate the battery charging stuff more cleanly from the SOC calculation.

Also applies to the functions below, in my opinion.

I have experimented quite a lot if it. I am open to but it into kalman_soc.h, but there might be problems with access to it from other places of the code. Best we do a confernce call to speak about it. Quite hard to explain in words.

martinjaeger · 2022-05-04T17:11:28Z

app/src/kalman_soc.cpp

+    float *tmp4;
+    float *tmp5;
+
+} EKF;


Ah, this looks very similar to the struct in bat_charger.h. Why do we need it two times?

I guess, the purpose is to have access to the data from other .c files, as we cannot use classes in C code. There is high provability that there are much easier solutions. Let's discuss it in a call as suggest in: https://github.com/LibreSolar/charge-controller-firmware/pull/130/files#r866892157

martinjaeger · 2022-05-04T17:20:31Z

tests/src/tests_kalman_soc.cpp

+    float tmp4[NUMBER_OF_OBSERVABLES_GPS][NUMBER_OF_OBSERVABLES_GPS];
+    float tmp5[NUMBER_OF_OBSERVABLES_GPS];
+
+} ekf_gps_t;


Is this struct not similar to one of the large structs previously defined? Could it be reused?

Never tried it. As mentioned in https://github.com/LibreSolar/charge-controller-firmware/pull/130/files#r866892157 I messed around a lot with this struct.
NUMBER_OF_OBSERVABLES_GPS, NUMBER_OF_OBSERVABLES_GPS would need to become variable or I would need to redefine it, which I remember should not be done.

Anyways the whole GPS Code is not needed long term. It is an example from TinyEFK library which I implemented as a test, to ensure that the library always behave correct, after code changes.

app/src/bat_charger.cpp

app/src/bat_charger.h

Added space after "," in function charger.update Co-authored-by: Martin Jäger <[email protected]>

Remove Space Between Comment and function

Remove unnecessary empty line

mulles

Reviewed most, a couple of comments might be missing. I don't seem fit with the review tools.

mulles · 2022-05-06T13:26:16Z

app/src/bat_charger.cpp

+            break;
+        }
+    }
+    return (voltages_size - index) * (soc_scaled_hundred_percent / voltages_size);


I guess, it is not important, as the initial SoC will be corrected by Kalman filter pretty fast.
Thus, it would not be my priority to implement this. You are free to improve it. The possible increase in CPU-cycles should be negligible.

app/src/bat_charger.cpp

mulles · 2022-05-06T14:35:21Z

app/src/bat_charger.h

+    float tmp4[NUMBER_OF_OBSERVABLES_SOC][NUMBER_OF_OBSERVABLES_SOC];
+    float tmp5[NUMBER_OF_OBSERVABLES_SOC];
+
+} EkfSoc;


I have experimented quite a lot if it. I am open to but it into kalman_soc.h, but there might be problems with access to it from other places of the code. Best we do a confernce call to speak about it. Quite hard to explain in words.

Name the correct function that has been replaced.

mulles

Now all files should got a review.
Moreover, I edited the code to reflect the comments. Expect for Interpolation question and EKF struct. Which the former is best discussed in a call.

mulles · 2022-05-10T09:39:23Z

Thanks a lot @martinjaeger for reviewing the PR.

The style should be improved now. Sorry for handling the review tools so badly, I hope it is not to chaotic.

Correct, currently it only works for 12V lead-acid batteries, but the code is ready for other chemistries. Minor adjustments would make it work, but it has only been tested for 12V lead-acid batteries.

The bigger problem is that I still have doubts concerning correctness of the equation system used. Thus I am working hard to try out other equation systems, which I have the correct derivation for.

Probably the project doesn't need any external code in future, as the understanding gets good enough to write it our self.

The option of a separate branch would be great to keep things organized and still visible.

mulles added 5 commits April 6, 2022 16:40

Pull Request SoC with Extended Kalman Filter

a0eec08

-Debugging is still done partially by printf, as I was not able to get the zephyr tool for it running. -The equation system for the model is probably incorrect, we should discuss it.

Pull Request SoC with Extended Kalman Filter

83326d9

-Debugging is still done partially by printf, as I was not able to get the zephyr tool for it running. -The equation system for the model is probably incorrect, we should discuss it.

Merge branch 'main' of github.com:mulles/charge-controller-firmware

8c865dc

Merge branch 'main' of github.com:mulles/charge-controller-firmware

855e7f7

Merge branch 'main' of github.com:mulles/charge-controller-firmware

a0cde84

martinjaeger reviewed May 4, 2022

View reviewed changes

mulles and others added 4 commits May 6, 2022 15:30

Code-Style Correction

47febf7

Added space after "," in function charger.update Co-authored-by: Martin Jäger <[email protected]>

Code-Style Correction - Removed Space

41579df

Remove Space Between Comment and function

Code-Style Correction - added empty line

9668833

Code-Style Correction

540232c

Remove unnecessary empty line

mulles commented May 10, 2022

View reviewed changes

Correction of a comments

e476f7e

Name the correct function that has been replaced.

mulles commented May 10, 2022

View reviewed changes

Pull Request SoC with Extended Kalman Filter #130

Are you sure you want to change the base?

Pull Request SoC with Extended Kalman Filter #130

Uh oh!

Conversation

mulles commented Apr 6, 2022

Uh oh!

martinjaeger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mulles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mulles left a comment

Choose a reason for hiding this comment

Uh oh!

mulles commented May 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants