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

Errors in documentation #431

Closed
lnlp opened this issue Sep 6, 2019 · 4 comments
Closed

Errors in documentation #431

lnlp opened this issue Sep 6, 2019 · 4 comments
Assignees
Labels

Comments

@lnlp
Copy link

lnlp commented Sep 6, 2019

I found the following errors and inconsistencies in the .docx/.pdf documentation:

ISSUE 1:

2.2.15 ostime_t us2osticks(s4_t sec)

should read:

2.2.15 ostime_t sec2osticks(s4_t sec)

ISSUE 2:

2.3.4 void **onEvent(ev_t ev)

"In any case, this interface is better handled by registering an event function at run time. See the discussion of LMIC_registerEventCb(), below."

An explanation for why it is better ('in any case') is missing. Why is it better?

'In any case better' but use of LMIC_registerEventCb() currently always requires the following to be defined (to disable the legacy onEvent() event handler):

    #define LMIC_ENABLE_onEvent 0

This dependency / requirement is undesired and can be prevented (see #433).
In fact it is currently not even documented for LMIC_registerEventCb() (see issue 8).

ISSUE 3:

2.5.12 void LMIC_setTxData_strict()

Lacks a clear description of what makes LMIC_setTxData_strict() different from LMIC_setTxData().
Such description should be put at the start.
The first paragraph in 2.5.12 is just a repetition of the first paragraph of 2.5.11 (but uses different words which makes it more difficult to compare). The repetition is redundant and unnecessary. Instead the first paragraph should be replaced with a description of what it is different from LMIC_setTxData().

"This API is preferred for new applications, for two reasons...." This is an advice to use the strict version but should not be a replacement for the lacking description at the start.

ISSUE 4:

2.5.15 lmic_tx_error_t ... the following values may be returned.

Description for value -3 should read "for any enabled data rate" instead of "for the current data rate".

ISSUE 5:

2.5.15 and 2.5.16

Documentation for LMIC_sendWithCallback() and LMIC_sendWithCallback_strict() does not make clear if the Callback is a replacement for the EV_TXCOMPLETE event or not.
I.e. will an EV_TXCOMPLETE event (if successful) also be generated or will only the callback be called and will te EV_TXCOMPLETE event not be generated?
(Not clear because it is not explicitly mentioned and because of inconsistency between descriptions in tables in 2.5.15 and 2.5.16).

ISSUE 6:

2.5.15 and 2.5.16

Depending on what is described at Issue 5 (will EV_TXCOMPLETE be generated or not?):

  • Either the descriptions in the table in 2.5.15 should mention EV_TXCOMPLETE,
  • or, EV_TXCOMPLETE should be removed from the description in the table in 2.5.16.

ISSUE 7:

2.5.15 lmic_tx_error_t ... the following values may be returned.

Table with lmic_tx_error_t values is already listed in section 2.5.13.
Repeating the table in 2.5.13 gives the impression that it may be different from the table in 2.5.13 which it actually is not. Comparing the descriptions in both tables shows they are not textually identical but they appear to mean exactly the same.
I would like to suggest to leave out the table in 2.5.15 and add a reference to the table 2.5.13 instead.
If I'm correct the only difference between 2.5.13 and 2.5.15 is the addition of a callback but no differences in the return values and their meaning.

ISSUE 8:

2.5.19 void LMIC_registerEventCb(...)

Lacks mentioning dependency / requirement:

    #define LMIC_ENABLE_onEvent 0

ISSUE 9:

2.5.19 void LMIC_registerEventCb(...)

The EV_LINK_DEAD section is listed twice.

Suggestion:

  • Adding a revision history chapter to the document would be useful.
@lnlp lnlp changed the title Error in documentation Errors in documentation Sep 6, 2019
@terrillmoore
Copy link
Member

Thanks for your careful review.

Issue 1. Agreed.
Issue 2. Docs can be improved, but the code does not require that you supply an onEvent function. See my comments under #433.
Issue 3. Agreed
Issue 4. Agreed
Issue 5. Agreed, will try to clarify.
Issue 6. Agreed.
Issue 7. Yes, we should avoid duplication of that kind.
Issue 8. See my comments under #433. It is not required. The only thing additional if you #define it is that a few bytes are saved.
Issue 9. Agreed.

Suggestion. Agreed; I have tools to automatically generate a decent Word redline, and I'll do that once the document stabilizes.

@terrillmoore terrillmoore self-assigned this Sep 10, 2019
@terrillmoore
Copy link
Member

I can't find the duplication of EV_LINK_DEAD.

@terrillmoore
Copy link
Member

Hi @lnlp -- because of the delay in making the release due to #635, there's a chance to review the changes, if you have time...

@lnlp
Copy link
Author

lnlp commented Dec 7, 2020

Hi @terrillmoore,
I'll have to skip it at the moment, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants