Olen / solar-monitor

GNU General Public License v3.0
54 stars 21 forks source link

Solar link batt temp #4

Closed oedo808 closed 2 years ago

oedo808 commented 2 years ago

Hi @Olen! I have a battery temperature monitor and noticed you were already grabbing the data from the GATT characteristics so I added some properties to the solardevice class and added it to the battery_temperature MQTT logger. Let me know if you want anything changed to this before accepting the PR or feel free to create one off of my branch to merge.

I've tested this on a Renogy Rover 40a: Renogy BT-1 module and it appears to be working great. My controller is always warmer than my temperature sensor and I'm showing a 7 degree Celsius difference.

Anto79-ops commented 2 years ago

so I also have the battery temperature monitor. At the moment, what temperature is it reading? would the new code now show Controller temp AND battery temp?

EDIT: i can confirm the changes that you are suggesting work for my Rover 40 BT-1. Battery temperature and rover temperature are now reporting.

oedo808 commented 2 years ago

Mine is reading 10 Celsius because it's cold outside and the sensor is waist height, my controller is 17 Celsius, I think because it's up high. I don't need it for the battery coefficient so I'm trying to see the general temp of my greenhouse.

I've also got the charge state going on another branch. I haven't submitted the PR yet because I think I broke charge power publishing, I just noticed this morning. I'm also getting the historical data, like daily AH charged and max watts.

If you want to create a develop branch I can submit the PRs to it instead of master so you can test on your end, suggest changes, etc.

On Fri, Jan 14, 2022, 5:07 PM Anto79-ops @.***> wrote:

so I also have the battery temperature monitor. At the moment, what temperature is it reading?

— Reply to this email directly, view it on GitHub https://github.com/Olen/solar-monitor/pull/4#issuecomment-1013532636, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXDOXNMHMHWRZGAJXXCUTTUWCUCHANCNFSM5L75RNLQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

Anto79-ops commented 2 years ago

Mine is reading 10 Celsius because it's cold outside and the sensor is waist height, my controller is 17 Celsius, I think because it's up high. I don't need it for the battery coefficient so I'm trying to see the general temp of my greenhouse. I've also got the charge state going on another branch. I haven't submitted the PR yet because I think I broke charge power publishing, I just noticed this morning. I'm also getting the historical data, like daily AH charged and max watts. If you want to create a develop branch I can submit the PRs to it instead of master so you can test on your end, suggest changes, etc. On Fri, Jan 14, 2022, 5:07 PM Anto79-ops @.***> wrote: so I also have the battery temperature monitor. At the moment, what temperature is it reading?

It's be very interested in knowing how you can get the charge state and Ah, max watts

Please do submit to separate branch!

oedo808 commented 2 years ago

I have all the daily stats logging now. I accidentally pulled from my battery temp branch so I'm working on rebasing it so that it can be merged separate from the battery temperature. Once I'm done with getting it to the main solarmonitor class and added to logging I'll submit a PR.

I have the current charging state working, AKA not charging, mppt charging, boost charging, etc. I just need to make one last change and I can submit a PR for it as well.

Next I hope to begin finishing the battery charging configuration parameters. I'm a little scared of writing to them and I think it's beyond the scope of this project but it would be nice to be able to verify that the horrible Android app is at least saving properly. I don't think this should be polled constantly, what do you think about receiving an MQTT command or maybe a config flag and reading then printing the values?

On Sat, Jan 15, 2022, 6:10 PM Anto79-ops @.***> wrote:

Mine is reading 10 Celsius because it's cold outside and the sensor is waist height, my controller is 17 Celsius, I think because it's up high. I don't need it for the battery coefficient so I'm trying to see the general temp of my greenhouse. I've also got the charge state going on another branch. I haven't submitted the PR yet because I think I broke charge power publishing, I just noticed this morning. I'm also getting the historical data, like daily AH charged and max watts. If you want to create a develop branch I can submit the PRs to it instead of master so you can test on your end, suggest changes, etc. … <#m7933752190223104050> On Fri, Jan 14, 2022, 5:07 PM Anto79-ops @.> wrote: so I also have the battery temperature monitor. At the moment, what temperature is it reading? — Reply to this email directly, view it on GitHub <#4 (comment) https://github.com/Olen/solar-monitor/pull/4#issuecomment-1013532636>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXDOXNMHMHWRZGAJXXCUTTUWCUCHANCNFSM5L75RNLQ https://github.com/notifications/unsubscribe-auth/AEXDOXNMHMHWRZGAJXXCUTTUWCUCHANCNFSM5L75RNLQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.>

It's be very interested in knowing how you can get the charge state and Ah, max watts

Please do submit to separate branch!

— Reply to this email directly, view it on GitHub https://github.com/Olen/solar-monitor/pull/4#issuecomment-1013779358, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXDOXJIH2FP4LLEKGRCUMTUWIEFZANCNFSM5L75RNLQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

Anto79-ops commented 2 years ago

that's awesome. I can't wait to get what I have now plus the missing parameters and charge state (mppt,boost, etc)

With regards to the configuration, personally, it doesn't matter to me only because they rarely change for me. I mean, you're, right, it would be beyond the scope of this project. At the very least, when the program first starts, it would be good for it to display the values are read-only.

Olen commented 2 years ago

Hi,

Just to let you know - I am extremely happy that my humble project is useful for others. And I am glad people are providing PRs.

I am going to merge the ones that looks fine, but I won't have time to actually test anything before I merge. So if anything breaks when you update, please submit another PR to fix them. My installation is 4 hours away in a place that is currently inaccsessible due to weather conditions, so I am not very keen on deploying stuff there that can potentially stop it from working.

oedo808 commented 2 years ago

Thanks @Olen! I am able to test that everything is working fine on Renogy but am unable to test to confirm my suspicions that Renogy is a rebranded SolarLink or vice versa. I'll only be submitting PR's that are tested and stable on my installation. I'll be keeping the individual features separate so if one breaks SolarLink it can be rolled back easily.

Would you want to create a separate potentially unstable branch, maybe develop, for me to submit these to so that you or others can switch back to master if something is incompatible? If I find that a feature is incompatible with SolarLink I will create a separate plugin for Renogy to ensure that the features that work with SolarLink are kept separate from the features that work with Renogy. If you prefer I can do this instead so that you or others using SolarLink can stay on the main plugin and potentially test features by switching plugins.

Olen commented 2 years ago

Hi, and thanks a lot for the PR(s).

As you probably have noticed, the whole code base could use some cleanup. Maybe some tests should be added, a separation between head and dev, and release-tags etc, etc... This was just a pet project for me, as I was sick of not having remote logging, and only a mobile GUI when I was on site, and I never really thought anyone would find it. And the all of a sudden I am starting to get bug reports and PRs. Which is great actually!

WIth that said, I think SolarLink is just some OEM, som there are probably lots of systems using the same hardware (and firmware). I have seen quite a few mobile apps that looks the same, maybe except for a logo and the color scheme, and are just clones of each other with a different brand name. So they are probably in reality the same. I had to reverse engineer the protocols based on the android app and packet captures, so there are probably some bugs in the parsing or some values I have misinterpreted. Go ahead and submit PRs. I'll ask if I don't understand what you do. Feel free to clean up the rest of the mess as well... If something breaks, when I update my own system, I'll let you know and we can split the two brands if they are incompatible. For now I think it is better to keep one "plugin" with the most features and fixes, as long as we can.