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

Fix typos, grammar, spelling and factual mistakes in the documentation #203

Merged
merged 11 commits into from
Aug 1, 2024

Conversation

IAmMarcelJung
Copy link
Collaborator

After stumbling over another small mistake in the docmentation, I decided to check the whole documentation using ltex. As the name implies, it is normally used for LATEX documents, but it can also be used for reStructuredText (and others). I did of course check the suggestions and only applied the ones that made sense to me. There is of course also no guarantee that every error was catched, but I am quite happy with what it found.

In simulation.rst there also was a factual error, stating that 0xFAB0 causes sampling the data and creating a one-cycle strobe, although actually 0xFAB1 causes it.

As always, any comments or suggestions are appreciated!

@IAmMarcelJung IAmMarcelJung marked this pull request as draft July 2, 2024 11:34
@IAmMarcelJung
Copy link
Collaborator Author

I found even more mistakes and will include them in another commit.

@IAmMarcelJung
Copy link
Collaborator Author

Although the generation runs without errors, the generated files seem to be broken. I will investigate it and remove the draft if it is solved.

@IAmMarcelJung
Copy link
Collaborator Author

I messed up my local sphinx install. After repairing it, I could generate valid HTML files again and view the results. The changes can now be reviewed.

@IAmMarcelJung IAmMarcelJung marked this pull request as ready for review July 2, 2024 21:53
@KelvinChung2000
Copy link
Collaborator

I will try to review them tomorrow.

Since you are working on the doc, it would be nice if you reviewed #197, which greatly enhances its appearance.

Probably not within this pull request, but it would also be nice if you could copy some of the yosys and nextpnr doc to FABulous doc to make FABulous more approachable for people not familiar with the Linux like #205

@IAmMarcelJung
Copy link
Collaborator Author

Thanks! I am not actively working on the documentation, this was mainly done for procastination...

However, I can still try improve the documentation and make it more beginner friendly. To my shame, I think I also did not document #138, so this also has to be done.

When I worked with the documentation, I noticed that the files are not formatted at all and some files had exceptionally long line, which made it a bit tedious to review them. Since we now use black as a formatter for python code, I looked for a restucturedText formatter. I only found rstfmt and docstrfmt. They both seem to be more or less active and the latter is influenced by the former. Should I open a discussion about if and which formatter to use?

@KelvinChung2000
Copy link
Collaborator

I also should be working on something other than FABulous at the moment... procrastination...

More formatting would be nice. Since you are on the boat now, you can make the decision. Better doc support is a continuous effort, so do them when you don't want to work 😉. To be honest, once we moved to pip setup, things should become simpler. If the setup tool can also install yosys and nextpnr, then we don't even need to make doc for them.

@IAmMarcelJung
Copy link
Collaborator Author

IAmMarcelJung commented Jul 3, 2024

Okay then I don't think it is necessary to document the installation of yosys and nextpnr any further.

I forgot to mention that I tried out docstrfmt, but it broke my HTML documents, although they were generated without any error. To get the generation working, I also had to change the sphinx version, so maybe it will also be fixed once #197 is done. I will definietly wait with the formatting until #197 and this PR are merged, since it might get confusing otherwise.

Good to know I'm not the only one who is procrastinating 😄

Copy link
Contributor

@TaoBi22 TaoBi22 left a comment

Choose a reason for hiding this comment

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

My obligatory drive-by of anything to do with docs :)

Thanks for looking at this!

docs/source/FPGA-to-bitstream/Nextpnr compilation.rst Outdated Show resolved Hide resolved
docs/source/FPGA-to-bitstream/VPR compilation.rst Outdated Show resolved Hide resolved
docs/source/fabric_definition.rst Outdated Show resolved Hide resolved
docs/source/fabric_definition.rst Outdated Show resolved Hide resolved
@IAmMarcelJung IAmMarcelJung marked this pull request as draft July 16, 2024 12:59
…, finish some TODOs, add shrink my FPGAs paper to the publications and cite it, also fix minor issues
@IAmMarcelJung
Copy link
Collaborator Author

The last commit (d7cf183) implements using the todo extension. Now no TODO is in the generated docs anymore.

Also two TODOs are finished:

  1. Cite the "Shrink my FPGAs" paper where it was appropriate (I asked Dirk for this)
  2. Add a description for SHARED_PORT. An additional example of a SHARED_PORT was added in both the Verilog and the VHDL examples (Dirk told me to do so and I also think it is useful).

Two "bug fixes: were made:

  1. Changed Out_Pad to eFPGA for at the end of the eFPGA entity.
  2. Add end entity LUT4 in the example in the Bitstream remapping chapter.

I also asked Dirk for the other TODOs, but I'm still waiting for the answer.

If anyone else knows what has to be written for the TODOs you can of course also let me know or directly change it (I think only short additions are needed. Thanks in advance!

@IAmMarcelJung
Copy link
Collaborator Author

In the latest commits, another TODO could be fixed (thanks to @EverythingElseWasAlreadyTaken!). Also a small formulation and punctuation fix was done.

@IAmMarcelJung
Copy link
Collaborator Author

In the last commit (0b7f590) just the hint on the emulation setup was updated and turned into a note, so it does not look anymore as if it was just some internal comment.

@IAmMarcelJung IAmMarcelJung marked this pull request as ready for review July 30, 2024 12:53
Copy link
Collaborator

@KelvinChung2000 KelvinChung2000 left a comment

Choose a reason for hiding this comment

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

Should I merge this first or #210 ? Since the conf.py might lead to merge conflict.

@IAmMarcelJung
Copy link
Collaborator Author

Will it also lead to conflicts if I revert the changes made by black? Other than that I just added the todo extension. I could just revert the formatting if that is enough to avoid conflicts.

@KelvinChung2000
Copy link
Collaborator

I thought this is going to development, never mind.

@KelvinChung2000 KelvinChung2000 merged commit e8e0c72 into FPGA-Research:master Aug 1, 2024
2 checks passed
@IAmMarcelJung IAmMarcelJung deleted the doc/minor_fixes branch August 1, 2024 11:51
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