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

Update list of tags and move to enum #87

Merged
merged 12 commits into from
Oct 11, 2023
Merged

Conversation

eikowagenknecht
Copy link
Contributor

@eikowagenknecht eikowagenknecht commented Sep 28, 2023

Implements #85

Moving to Enum instead of loose string references has the advantage that now only existing tags can be used and the IDE picks up on errors.

Unfortunately since "BAT_REQ_MEASURED_RESISTANCE" (0x03000130) and "BAT_RUN_MEASURED_RESISTANCE" (0x03800131) don't exist any more, I had to remove those two from the output.

But I think this is a good thing because those two were wrong anyways (see values in screenshots):

  • 0x03000130 reported the avg30 current (this is not used any more, but there is 0x03800130 = BAT_INTERNAL_CURRENT_AVG30)
  • 0x03800131 reported the avg30 voltage (this is now called BAT_INTERNAL_MTV_AVG30)

I can add those avg30s values to the battery results if you want. Currently those only exist on DCB level.

image

@vchrisb
Copy link
Collaborator

vchrisb commented Oct 2, 2023

Thank you @eikowagenknecht! 🥇
That is a lot of change.
Can you please separate the two commits into two PRs?

@eikowagenknecht
Copy link
Contributor Author

Ooops, I intended to do so. The other PR should be fine, but this one needs to be rebased against master. On it.

@eikowagenknecht
Copy link
Contributor Author

Done. I also fixed the flake8 errors.

Signed-off-by: Eiko Wagenknecht <[email protected]>
@eikowagenknecht
Copy link
Contributor Author

Fixed the remaining two flake8 errors..

@eikowagenknecht
Copy link
Contributor Author

Ran isort and black as well now...

Copy link
Collaborator

@vchrisb vchrisb left a comment

Choose a reason for hiding this comment

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

please add Args and Returns to the docstring.

e3dc/_rscpTags.py Outdated Show resolved Hide resolved
e3dc/_rscpTags.py Outdated Show resolved Hide resolved
e3dc/_rscpTags.py Outdated Show resolved Hide resolved
e3dc/_rscpTags.py Outdated Show resolved Hide resolved
e3dc/_rscpTags.py Outdated Show resolved Hide resolved
e3dc/_rscpTags.py Outdated Show resolved Hide resolved
@vchrisb
Copy link
Collaborator

vchrisb commented Oct 5, 2023

@eikowagenknecht do you potentially already created a script how you generated the RscpTag?
I think it would be valuable to add this somewhere to make future updates easier.

@vchrisb
Copy link
Collaborator

vchrisb commented Oct 5, 2023

  • would you mind moving rscpErrorCodes and powermeterTypes also to enum?
  • may I suggest using rscptype instead of type_?

@eikowagenknecht
Copy link
Contributor Author

@eikowagenknecht do you potentially already created a script how you generated the RscpTag? I think it would be valuable to add this somewhere to make future updates easier.

Sure, I commented and added the script. I put it into a newly created tools directory, if you want it somewhere else I'm happy to move it.

@eikowagenknecht
Copy link
Contributor Author

  • would you mind moving rscpErrorCodes and powermeterTypes also to enum?
  • may I suggest using rscptype instead of type_?

All done.

@eikowagenknecht
Copy link
Contributor Author

Seems like flake8-docstrings is used as well in the GitHub workflow. I added that requirement to the developer section of the readme.

Copy link
Collaborator

@vchrisb vchrisb left a comment

Choose a reason for hiding this comment

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

We are getting close to merging this! 💯
Thank you for sticking to it! 🥇

tools/_convert_rscp_tags.py Outdated Show resolved Hide resolved
tools/_convert_rscp_tags.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
e3dc/_rscpLib.py Outdated Show resolved Hide resolved
@vchrisb vchrisb merged commit fd7bd95 into fsantini:master Oct 11, 2023
5 checks passed
@fsantini
Copy link
Owner

Congratulations :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants