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

docs: man/man1/*.adoc - review asciidoc formatting (smoe:docs_wording_20240807) #3060

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

smoe
Copy link
Contributor

@smoe smoe commented Aug 10, 2024

Likely completed review of man/man1.

The second patch was a series of harmless simplifactions for the translations that were still in my git stash.

Copy link
Member

@hansu hansu left a comment

Choose a reason for hiding this comment

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

Thanks for continuing on this!
a2x seems to be a bit buggy in our case. I haven't had the time yet to investigate this further. See #3057
Removing the square bracket changes in the two files and the other one thing makes the build run successfully on my machine.

@petterreinholdtsen
Copy link
Collaborator

I do not understand the rationale behind all your changes, but have no objections to them. Most of the changes make sense to me, but some I suspect neither affect the layout nor the po4a splitting of strings. This is just meant as a suggestion for you to make sure the changes do what you believe they are doing, not as a request to change anything in the patches unless you yourself believe they are wrong.

@smoe
Copy link
Contributor Author

smoe commented Aug 11, 2024

@hansu, the []s are important, pointing to a section of the INI file. I found a small error that with a bit of luck fixes it.
@petterreinholdtsen, yes, I agree, some changes I only made in anticipation of problems with po4a, but I expect also the human eyed to benefit from them, and others just for cosmetic purposes that would not affect the HTML or pdf representation. And sometimes I did not think about what I was doing, it just felt right.

@petterreinholdtsen
Copy link
Collaborator

The failing CI test seem to be a problem with Debian unstable (perhaps gcc 14), not with the adoc changes.

_VFD_NAME_**.stat-bit-0** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-1** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-2** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-3** (bit, out):: Raw status bit. Set the VFD so this is motor-at-speed status. FIXME: Unclear - is the signal set or is the wiring to be set in a particular way?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could some good soul please help with a patch that improves the wording of this description?

Copy link
Member

Choose a reason for hiding this comment

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

You have to configure the vfd so that this status bit is mapped to the function "up to frequency"

See https://dl.mitsubishielectric.com/dl/fa/document/manual/inv/sh060014/sh060014engb.pdf

page 458 and pages 61/62:

grafik

grafik

grafik

_VFD_NAME_**.stat-bit-4** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-5** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-6** (bit, out):: Raw status bit
_VFD_NAME_**.stat-bit-7** (bit, out):: Raw status bit. Set the VFD so this in the alarm bit. FIXME: Unclear - is the signal set or is the wiring to be set in a particular way?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, I did not get it.

@smoe
Copy link
Contributor Author

smoe commented Aug 13, 2024

The "fixme"s I came up with aside, I think this can be accepted and the PR closed.

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Aug 14, 2024 via email

@hansu
Copy link
Member

hansu commented Aug 14, 2024

Let me test it with a local build to see if the square brackets are all formatted properly.

@hansu
Copy link
Member

hansu commented Aug 16, 2024

The failing CI test seem to be a problem with Debian unstable (perhaps gcc 14), not with the adoc changes.

Sure that this a a CI thingy?

Runtest: 271 tests run, 269 successful, 2 failed + 0 expected, 4 skipped
Failed: 
	./tests/mdi-queue/oword-queue-buster
	./tests/mdi-queue/simple-queue-buster

The result of the first looks like this

trying to connect to linuxcncrsh TOGO=80
Machine configuration file is 'linuxcncrsh-test.ini'
check_config validation failed
LinuxCNC terminated with an error.  You can find more information in the log:
    /home/cnc/linuxcnc_debug.txt
and
    /home/cnc/linuxcnc_print.txt
as well as in the output of the shell command 'dmesg' and in the terminal
trying to connect to linuxcncrsh TOGO=79
trying to connect to linuxcncrsh TOGO=78
trying to connect to linuxcncrsh TOGO=77
trying to connect to linuxcncrsh TOGO=76
...
trying to connect to linuxcncrsh TOGO=4
trying to connect to linuxcncrsh TOGO=3
trying to connect to linuxcncrsh TOGO=2
trying to connect to linuxcncrsh TOGO=1
connection to linuxcncrsh timed out

@hansu
Copy link
Member

hansu commented Aug 16, 2024

@smoe I wanted to push some changes on your branch, but it was rejected - did you disable
grafik
?

@smoe
Copy link
Contributor Author

smoe commented Aug 16, 2024

Have not knowingly disabled it but you are right, it was disabled when I just looked. Fixed that.

@smoe
Copy link
Contributor Author

smoe commented Aug 16, 2024

[Steffen Möller]
The "fixme"s I came up with aside, I think this can be accepted and the PR closed.
I had a look and sampled the change, and it look good to me. I have no idea about improved wording of the confusing parts. I did not merge it, in the hope that Andy or someone else might have wording improvements to contribute before it is merged. Perhaps some of the open conversations can be closed in this pull request, to make their status easier to grasp? -- Happy hacking Petter Reinholdtsen

@andypugh , may I kindly ask to have a quick look at those FIXMEs on https://github.com/LinuxCNC/linuxcnc/pull/3060#:~:text=View%20reviewed%20changes-,docs/src/man/man1/mitsub_vfd.1.adoc,-VFD_NAME**.stat%2Dbit%2D4 ?

@hansu
Copy link
Member

hansu commented Aug 16, 2024

Have not knowingly disabled it but you are right, it was disabled when I just looked. Fixed that.

thanks

@smoe
Copy link
Contributor Author

smoe commented Aug 30, 2024

Just rebased to master and squashed it all. Hansu's changes are still in :-)

@petterreinholdtsen
Copy link
Collaborator

Should this wait for better wording on the unclear VFD settings?

@hansu
Copy link
Member

hansu commented Sep 1, 2024

See #3060 (comment)

Copy link
Member

@hansu hansu left a comment

Choose a reason for hiding this comment

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

I would suggest something like this

@petterreinholdtsen
Copy link
Collaborator

Do you plan to rebase and squash again? I notice there are three commits, and would expect only one if it is squashed.

Series of smallish changes of no particular importance, except that is
likely helps with asciidoc simplifications in the translations, mostly
on white space.

Also clarified wording wrt mitsub_vfd .

Co-authored-by: Hans Unzner <[email protected]>
@smoe smoe force-pushed the docs_wording_20240807 branch from 78ff054 to c466813 Compare September 1, 2024 11:17
@smoe
Copy link
Contributor Author

smoe commented Sep 1, 2024

Do you plan to rebase and squash again? I notice there are three commits, and would expect only one if it is squashed.

Squashed.

@petterreinholdtsen petterreinholdtsen merged commit f4b383a into LinuxCNC:master Sep 1, 2024
9 checks passed
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