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

Documentation updates / Refactored Readme #182

Merged
merged 3 commits into from
Apr 24, 2023

Conversation

MalteSchm
Copy link

I took a stab at refactoring the readme. This PR:

  • Extends the hardware section to cover the used hardware in this project
  • Moves the hardware and flashing text into a separate document (because it's getting too much info)
  • Integrates MQTT, WebAPI, PinMapping information currently present in the readme into the respective (existing) documents
  • The existing Readme has been renamed to README_openDTU

Feedback is welcome

…ent. Integrated MQTT, Webapi, PinMappings related text in the correct sections. Updated documentation / diagrams to reflect the hardware required
@madmartin
Copy link

madmartin commented Apr 22, 2023

When you rename the original project's readme then @helgeerbe will have continuous merge conflicts when he merges the upstream master.
IMHO it is better to keep the original readme, just put a note in big letters at the top that the sub-project extra readme has a different name (and add a link to it).
The same applies to the images in the docs directory. Your concept is to replace several files, which I would not do. I would place this project's files with different filenames and reference them in this project's own readme.

@berni2288
Copy link

When you rename the original project's readme then @helgeerbe will have continuous merge conflicts when he merges the upstream master.

I have to agree here. I've justed tested out locally what would happen, when we rename the original README.md from OpenDTU and I got merge conflicts after doing a test merge with file changes in the upstream branch.
So creating our own README-OpenDTU-OnBattery.md and placing a link in the original README.md at top seems to be the best option like @madmartin says.

@madmartin
Copy link

madmartin commented Apr 22, 2023

I looked at the outcome of your changes https://github.com/MalteSchm/OpenDTU-OnBattery/tree/readme_refactor and cant find your new drawings - you removed the links from readme?!?
The pictures are nice, the deserve to be shown.

@MalteSchm
Copy link
Author

Ok I'll update this in a few minutes

@madmartin The schematics have been moved to the hardware page: https://github.com/MalteSchm/OpenDTU-OnBattery/blob/readme_refactor/docs/hardware_flash.md

@MalteSchm
Copy link
Author

Ok. I reverted the readme and moved the text to a new document

@helgeerbe
Copy link
Collaborator

Hi Malte, I apreciate your work. What is the state of this PR?

@MalteSchm
Copy link
Author

Hi @helgeerbe
This PR is completed and I don't plan further changes unless somebody finds a bug.

The only thing I consider as a TODO is writing some text around the Power Limiter. I did not do this so far as there is the other Power Limiter related change(s). If this gets accepted I would feel responsible / volunteer to fix this. But I would focus on the implementation first.

@helgeerbe helgeerbe merged commit 8f386c8 into hoylabs:development Apr 24, 2023
@helgeerbe
Copy link
Collaborator

Hi @MalteSchm I merged it into development.

There are a couple of things I'm not quite happy this.
openDTU is the core and everything that is related to the core, should be in the core documentation (e. g. the list of supported inverters). Otherwise wie have to maintain also this documentation.
An other example is the newly introduced CMT2300A module. This is missing in your documentation.

Actual I don't know what is the best way. As long as openDTU is quite volatile we should keep the doc separated. As soon, as openDTU becomes stable and there are only few changes. We can think about of merging the docs together.

@MalteSchm MalteSchm deleted the readme_refactor branch April 24, 2023 18:09
@MalteSchm
Copy link
Author

MalteSchm commented Apr 24, 2023

@helgeerbe
Well I think that we could remove duplicated text keeping the section headings only and then add a reference to the main openDTU doc in that section.
I could see that working for text where there are no / not a lot of -onBattery related changes. In other areas it's probably best to keep the text in the -onBattery responsibility as it becomes unreadable otherwise.
This approach would not prevent merge issues but makes things simpler in my mind

BTW: The CMT2300A module is actually included in the text. It is not part of the schematic however because there is no blueprint from the openDTU project yet.

Let me know what you think

Copy link

github-actions bot commented Apr 7, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants