Improve description of cmderr "busy" #1076
Conversation
|
See #1072 for the related discussion. |
There was a problem hiding this comment.
Changes are in line with ticket #1072 comments and aligned to expectations after discussing with Jan.
xml/dm_registers.xml
Outdated
| {abstractcs-cmderr} to be set to 1 (busy) if it is 0. | ||
| {abstractcs-cmderr} to become 1 (busy) once the command completes | ||
| (once {abstractcs-busy} becomes 0). |
There was a problem hiding this comment.
I'm on the fence about "become 1" while "becomes 0" reads OK to me. Not sure why but I wanted to point it out. Thoughts?
There was a problem hiding this comment.
Is this a normative change? It previously said that if you accessed these registers while busy=1 then cmderr would immediately become 1 if it was 0 (and if it was already non-zero then it would retain that value). Now it says that cmderr will become 1 at a later time (one normative change) regardless of the previous value of cmderr (another change).
(For what it's worth, "causes X to become" seems like correct grammar to me.)
There was a problem hiding this comment.
The definition of abstractcs.cmderr says: This field only contains a valid value if busy is 0.
In another words, the value of cmderr is undefined while busy==1.
For that reason, IMO:
-
It does not make sense to say
abstractcs.cmderr becomes 1 if it was 0because the value of cmderr is undefined while the command executes. For that reason, I would remove all the sentences from the spec that say this. -
It does not matter when abstractcs.cmderr is set to 1 (if immediately or at a later time). It is only required that it happens no later than when abstractcs.busy becomes 0. I have updated all the sentences to not imply when the change of cmderr is supposed to happen, only what the value should be when the command completes.
Please, let me know if the proposed wording looks clear and accurate to you now.
debug_module.adoc
Outdated
| If the debugger attempts to start a new command while {abstractcs-busy} is set, | ||
| the new command will not get started and the currently executing command still | ||
| gets to run to completion. After that, {abstractcs-cmderr} becomes 1 (busy), | ||
| and any error generated by the completed command is lost. |
There was a problem hiding this comment.
So the meat of this change is to move this into its own paragraph so that it's more obvious to a reader that this is the behavior no matter how an abstract command is started? (Can you start one without writing to dm.command?) Did I get that right?
There was a problem hiding this comment.
As for the whole PR -
The main reason for this PR was to clarify when abstractcs.cmderr shall become set to busy as there are currently sentences that imply different timing (when the command completes vs. immediately). Please see the thread above.
As for this concrete paragraph -
- I wanted present the information to the reader in a more natural order:
- i) specify how abstract commands are started,
- ii) specify how command failures are reported,
- iii) only after that, describe the special case of starting a second abstract command while the previous one is still running.
- The fact that the first abstract command continues to run is already mentioned in the current text. I added information that the second abstract command will not get started
There was a problem hiding this comment.
rtwfroody: Can you start [an abstract command] without writing to dm.command?
Yes - by the means of dm.abstractauto. However, that was not the main motivation for moving this paragraph down (see my previous reply).
- Improve the wording of what happens if another abstract command is started while the previous one still executes. Make the order of events very clear. - Unify the wording of cmderr "busy" requirement for all concerned DM registers. Make it clearer to the reader that the prescribed behavior is the same in all the cases.
JanMatCodasip
force-pushed
the
improve-busy-cmderr
branch
from
8a5764c to
f75e3fc
Compare
Improve the wording of what happens if another abstract command is started while the previous one still executes. Make the order of events very clear.
Unify the wording of cmderr "busy" requirement for all concerned DM registers. Make it clearer to the reader that the prescribed behavior is the same in all the cases.