search

openclimatefix / open-source-quartz-solar-forecast

Open Source Solar Site Level Forecast
MIT License
69 stars 55 forks source link

Add Victron inverter #202

Closed mduffin95 closed 3 weeks ago

mduffin95 commented 1 month ago

Pull Request

Description

Adds support for Victron inverters.

How Has This Been Tested?

I've got access to a Victron system so I've been able to test it against their API. I've also included some response data from their API and used it to write a small test.

Checklist:

peterdudfield commented 1 month ago

hi @mduffin95

Thanks so much for this. Really appreciate you putting in the effort to put this in. @zakwatts or @aryanbhosale do you fancy reviewing this?

Thanks

aryanbhosale commented 1 month ago

hi @mduffin95

Thanks so much for this. Really appreciate you putting in the effort to put this in. @zakwatts or @aryanbhosale do you fancy reviewing this?

Thanks

LGTM! @mduffin95 would be nice if you could update the .env.example file from this PR

mduffin95 commented 1 month ago

Sorry for the late response, I've been away.

@aryanbhosale I've updated the pr with your suggested change

aryanbhosale commented 1 month ago

Sorry for the late response, I've been away.

@aryanbhosale I've updated the pr with your suggested change

Thank you, @peterdudfield i guess this could be merged now

peterdudfield commented 1 month ago

Sorry for the late response, I've been away. @aryanbhosale I've updated the pr with your suggested change

Thank you, @peterdudfield i guess this could be merged now

Thanks, yea just one comment, we are trying to work out, about this repo - https://github.com/mduffin95/vrm-api-python-client

aryanbhosale commented 1 month ago

Sorry for the late response, I've been away. @aryanbhosale I've updated the pr with your suggested change

Thank you, @peterdudfield i guess this could be merged now

Thanks, yea just one comment, we are trying to work out, about this repo - https://github.com/mduffin95/vrm-api-python-client

Yea i think opening an Issue and a PR in their original repo is a good idea, but i doubt theyll respond since the last activity was an issue that was opened 2 years back

peterdudfield commented 1 month ago

Sorry for the late response, I've been away. @aryanbhosale I've updated the pr with your suggested change

Thank you, @peterdudfield i guess this could be merged now

Thanks, yea just one comment, we are trying to work out, about this repo - https://github.com/mduffin95/vrm-api-python-client

Yea i think opening an Issue and a PR in their original repo is a good idea, but i doubt theyll respond since the last activity was an issue that was opened 2 years back

Hm,yea, and it does say its not maintained. maybe some different options

  1. Create PR into orginal repo from https://github.com/mduffin95/vrm-api-python-client
  2. Create a vrim-api-python-client in OCF repos and publish to pypi
  3. Pull in relevant code to this repo, perhaps we dont actually need that much

I would defiantly do 1. so other people can see the improvements. Im not sure about 2. or 3., probably depends how much code is needed for 3.

I think separately we should put this a an optional requirements in the pyproject.toml as most people dont need to install this.

mduffin95 commented 1 month ago

@peterdudfield @aryanbhosale

The original repo hasn't accepted any new PRs for years so I don't think 1 will work. IMO 2 is a better option than 3 for a couple of reasons:

I personally think it's better to reduce the scope of this repo to not include too much low-level API connectivity code. It keeps this repo clearly focussed on forecasting.

I think separately we should put this a an optional requirements in the pyproject.toml as most people dont need to install this.

☝️ I can make this change now.

aryanbhosale commented 1 month ago

@peterdudfield @aryanbhosale

The original repo hasn't accepted any new PRs for years so I don't think 1 will work. IMO 2 is a better option than 3 for a couple of reasons:

  • It allows others to use a working python client for the Victron API for other purposes
  • It minimises the amount of vendor-specific code within this repo

I personally think it's better to reduce the scope of this repo to not include too much low-level API connectivity code. It keeps this repo clearly focussed on forecasting.

I think separately we should put this a an optional requirements in the pyproject.toml as most people dont need to install this.

☝️ I can make this change now.

Sounds good to me

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.28%. Comparing base (5093843) to head (ec2da9d). Report is 18 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #202 +/- ## ========================================== + Coverage 61.76% 62.28% +0.51% ========================================== Files 25 25 Lines 1020 1034 +14 ========================================== + Hits 630 644 +14 Misses 390 390 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

peterdudfield commented 4 weeks ago

Sounds. I'll create a new repo and @mduffin95 I'll make you to admin of it. I'm happy to do the pypi project things if you get the code ready

peterdudfield commented 4 weeks ago

Sounds. I'll create a new repo and @mduffin95 I'll make you to admin of it. I'm happy to do the pypi project things if you get the code ready

Its created here https://github.com/openclimatefix/vrm-api-python-client. @mduffin95 ive invited you to be an admin on this repo. Do you want to do any changes, and I can then sort the pypi stuff out

peterdudfield commented 3 weeks ago

Great stuff, thanks @mduffin95 for this. I miht merge into a development branch, to make sure all tests work e.t.c