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

Added documentation for TM1637, TM1638 and MAX7219 Seven-Segment Display #640

Merged
merged 30 commits into from
Mar 24, 2021

Conversation

ajithvasudevan
Copy link
Contributor

@ajithvasudevan ajithvasudevan commented Feb 20, 2021

This PR adds TM163x.md ( and related changes to Displays.md, Commands.md, and API.md ), the documentation for the TM1637, TM1638 and MAX7219 Seven Segment Display support for Tasmota (arendst/Tasmota#10889 , arendst/Tasmota#11031 , arendst/Tasmota#11387)

@ajithvasudevan
Copy link
Contributor Author

The same driver (TM1637) would be modified for adding support for TM1638 as well as the MAX7219 seven-segment display.
PR is already submitted for TM1638, and the changes for the MAX7219 are also ready.

This means, this documentation will have to undergo changes to incorporate TM1638 and MAX7219 shortly (hopefully!).

So, question to whoever is in charge of reviewing/merging the documentation (@blakadder ?) -
Would you like me to make those changes right away, before you merge this?

@blakadder
Copy link
Contributor

i'll just wait till you decide its complete

@ajithvasudevan
Copy link
Contributor Author

i'll just wait till you decide its complete

Alright then. I will let you know when.
Meanwhile, it would be great if you can have a look at what's done already and tell me if something needs fixing.

Thank you :)

@barbudor
Copy link
Collaborator

@ajithvasudevan
There quite a lot of changes there
Have you validated that nothing is broken by generating locally the docs site with mkdocs ?

@ajithvasudevan
Copy link
Contributor Author

@barbudor ,

Yes, I just did, found a couple of breakages, and fixed them.

@barbudor
Copy link
Collaborator

Thanks @ajithvasudevan

Regarding the presentation of commands descriptions in the TM1637.md page, I'm not a big fan of the pictures aligned on the same line as the text:
image
May be a :<br> like below would look nicer ?
image
May be same for the picture in the "Features" list.

No other comments but the edit on Commands.md adds quite a lot of commands that are very specific to the TM1367.
I understand its good for people to be able to do a global search on the Commands page to find for a given command but I'm not a big fan of duplicating info and I think detailed doc a very specific topic are better located in a specific page.
May be having only a short sentence in Commands pointing to the detaills in TM137 page would be more efficient ?
Just my 2c, I will let @blakadder comment on that and decide for the merge.

@ajithvasudevan
Copy link
Contributor Author

ajithvasudevan commented Mar 13, 2021

@barbudor

May be a :<br> like below would look nicer ?

Yes, that looks nicer. I've prefixed :<br> for the in-line images.

May be having only a short sentence in Commands pointing to the detaills in TM137 page would be more efficient ?

I debated this myself, but decided in favor of the current edit to Commands.md as these would be the same commands for the TM1638 (PR submitted) as well as the MAX7219 (ready to submit PR). So it won't be all that specific.

I can still change it as per your suggestion, if you insist.

@barbudor
Copy link
Collaborator

I won't insist. I was just sharing my thoughts 😊
Blakadder takes the final decision

@ajithvasudevan
Copy link
Contributor Author

@blakadder ,
The documentation is now complete.
Please let me know if anything needs to be changed.

@ajithvasudevan ajithvasudevan changed the title Added documentation for TM1637 Seven-Segment Display Added documentation for TM1637, TM1638 and MAX7219 Seven-Segment Display Mar 20, 2021
@blakadder blakadder merged commit e8277db into tasmota:development Mar 24, 2021
@blakadder
Copy link
Contributor

Thanks for all the work!

@ajithvasudevan
Copy link
Contributor Author

Thanks for all the work!

It is my pleasure!

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