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

[Request] Validate usage of SunPosition.isSunsetAvailable() #1407

Closed
AndreasBoehm opened this issue Nov 19, 2024 · 6 comments · Fixed by #1448
Closed

[Request] Validate usage of SunPosition.isSunsetAvailable() #1407

AndreasBoehm opened this issue Nov 19, 2024 · 6 comments · Fixed by #1448
Labels
enhancement New feature or request

Comments

@AndreasBoehm
Copy link
Member

Is your feature request related to a problem? Please describe.

When i implemented the use of SunPosition.isSunsetAvailable() I assumed that this reports if the time of the ESP was set and is able to report day/night.

As it turns out this actually reports if sunset is a thing in the current timezone or if it never gets dark during the 'night'.

Describe the solution you'd like

  1. Document what SunPosition.isSunsetAvailable() actually returns.
  2. Check usages of SunPosition.isSunsetAvailable() and make sure its correctly used.

Describe alternatives you've considered

No response

Additional context

Thomas explained pretty well what this method actually tells us: tbnobody#2198 (comment)

@AndreasBoehm AndreasBoehm added the enhancement New feature or request label Nov 19, 2024
@stefan123t
Copy link

@AndreasBoehm

there is a already a comment in SunPosition.cpp

https://github.com/hoylabs/OpenDTU-OnBattery/blob/61aa32ab5e5d0bfd876a4b008342c6b54f82d0d0/src/SunPosition.cpp#L110,L118

Wouldn‘t that be sufficient ?

If sunrise / sunset calculation fails, eg. too far north / south the sunset functionality is not available.

@AndreasBoehm
Copy link
Member Author

The problem is that this comment is not on the getter and thats also the reason why i interpreted the result wrong.

@stefan123t
Copy link

Agreed, how do we document such details normally, do we use some kind of documentation automation like doxygen ?

@spcqike
Copy link

spcqike commented Nov 19, 2024

Do you guys use vscode? If so you could use jsdoc to give the function a proper description and explanation which should be shown by intellisense whenever you use the function

https://dev.to/sumansarkar/how-to-use-jsdoc-annotations-with-vscode-for-intellisense-7co

@stefan123t
Copy link

@spcqike sounds like a good plan for the Vue.js code of the Web App.

We could use JSDoc for the Web App and Doxygen for the C++ code.

Here is a Stack Overflow Entry to use Doxygen for both Generate JavaScript documentation with Doxygen
The additional benefit would be only one documentation for both parts of the code being produced by one generator.

@AndreasBoehm sorry to sort of high-jack your issue for enhancing the documentation / auto-generate docs:

I have also tried to write an Swagger / OpenAPI specification for OpenDTU in tbnobody#2410
You may find my first stab at the OpenDTU-OnBattery REST API specs here: tbnobody#378 (comment)

It would be great if we could auto-generate these API specs from the REST API inline documentation too.
But apparently the Feature Request for Doygen to support OpenAPI is still open: Compatibility with or support for REST APIs swagger / openAPI

@schlimmchen
Copy link
Member

Guys, please relax. Machine-readable code documentation is close to the end of the "maybe-we-do-that-if-we-ever-find-the-time-wishlist".

Andreas' added comment is important here because this is "hard" to wrap your head around this.

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

Successfully merging a pull request may close this issue.

4 participants