puzzling tip in millis() documentation

[Szepi]

The millis() documentation, on page https://www.arduino.cc/en/Reference/Millis, says: "Tip:

Note that the parameter for millis is an unsigned long, errors may be generated if a programmer tries to do math with other datatypes such as ints. " This makes no sense at all: millis() has no arguments! The tip() should be removed ASAP.

[ElectricRCAircraftGuy]

They're talking about the output parameter. The returned value is unsigned long...and what happens if a beginner writes "int totalTime = millis()*arrayLength"? Answer: math errors due to overflow on a signed int.

[Szepi]

Sure, they could add such a "tip" to every function returning a value.. At minimum the tip should not mention parameters. This is extremely confusing for any beginner if you ask me, even worse than not having the tip.

[Chris--A]

I can see how its confusing, parameter is used in an incorrect context.

I would change it to read:

Note: the return value for millis() is an unsigned long, logic errors may occur if a programmer tries to do arithmetic with smaller data types such as int's. Even signed long may encounter errors as its maximum value is half that of its unsigned counterpart.

[ElectricRCAircraftGuy]

@Chris--A +1

[Szepi]

Better indeed. Another idea: Why not just say that the usual rules of finite precision integer arithmetic apply? On the specific suggestion: I would not mention specifically ints or smaller data types: C/C++ will use promotion etc. The story is complicated. The best would be to link some documentation on this from the Arduino website (or even outside?); I tried to find some appropriate page but I could not find it (though maybe it exists).

[ElectricRCAircraftGuy]

saying "the usual rules of finite precision integer arithmetic apply" doesn't mean anything to a beginner. Might as well say nothing in that case, but something should probably be said.

I agree with linking to outside documentation. That's a good idea.

But what about this too: Note: the return value for millis() is an unsigned long. Numerical errors may occur if a programmer tries to store a millis() timestamp into a data type smaller than unsigned long, or if they do arithmetic with an unsigned long then store the result into a variable of a smaller data type such as int. Even signed long may encounter errors as its maximum value is half that of its unsigned counterpart.

Or: Note: the return value for millis() is an unsigned long. Numerical errors may occur if a programmer tries to store a millis() timestamp into a data type smaller than unsigned long, or if they do arithmetic with a variable of a larger datatype than that into which they store the result (ex: int a = b*c is a bad idea if b or c is a larger data type than int). Also keep in mind that you may not get what you expect when using signed long to store millis() timestamps, as its maximum value is half that of unsigned long.

I dunno. I think I like the last one the best.

[Szepi]

How about this:

Note: The return value for millis() is an unsigned long and the actual values can be quite large, and in particular may not fit into, say, an int. Programmers (as usual) should be careful with storing and manipulating the returned value, see, e.g., http://blog.regehr.org/archives/721 for the quirks of integer arithmetic.

Btw, I don't care about what would be mentioned, the link above is a fun quiz that many people may find useful.

On the example mentioned, i.e., that int a = b*c is a bad idea. The variables b,c can actually hold small values in which case this is fine. So I would not say it categorically that this is always bad.

[shiftleftplusone]

IMO the variable type of the return value should always be defined in the function definition; additionally, mentioning the exact-width integer types would be appreciated.

A good way for referencing, cross-referencing, layout, and source code examples is this cplusplus repository IMO http://www.cplusplus.com/reference/ctime/clock/ http://www.cplusplus.com/reference/ctime/time/

unsigned long     millis()

Description
Returns the number of milliseconds (unsigned long 32 bit value, alias: uint32_t) since the Arduino board began running the current program. 
This number will overflow (go back to zero), after approximately 50 days.

Parameters
None  (void)

Tip:
Note that the return value for millis()  is a 32-bit unsigned long (uint32_t) , 
calculation  errors may be generated if a programmer tries to do math with other datatypes such as 16-bit int. "
(edited)

[systronix]

@VogonJeltz +1 "IMO the variable type of the return value should always be defined in the function definition;" agreed! Specify return type always. Your description is much more clear. This principle could be applied to much of the arduino documentation. If there was a way to help do that I would be happy to help. A good example of clear documentation are the java language docs.

[Szepi]

I cannot agree more either. The only reason I did not suggest this is because I assumed people want to the documentation consistent (and it would be a lot of work to redo the whole documentation). But I definitely agree! Still, I am worried about the formulation of the note. An unspecified "error" is totally scary. What error? Will my Arduino stop working? We should be more specific, while keeping the documentation concise. Also, most of the time there will be no error generated. Also, there are many many other places that this warning could be mentioned - again, why here? Why only here? Should not the Arduino page have some gentle introduction to issues that arise when working with finite precision numbers?

[Chris--A]

The error distinction should be made.

There is a clear difference between a syntax error and logic error, error by itself is a bit ambiguous.

And true, there is no diagnostic for logic errors.

[Szepi]

Here is a link to a chapter in a book which describes rules of arithmetic in C. While it is not ideal to include an external reference (the problem is that of course Arduino uses a particular version of gcc, which will have a particular set of rules), maybe it is better than nothing. Btw, the reason it may be a bit complicated to include type declarations in the documentation is because the types may depend on the target platform and this has to be carefully checked.

[Szepi]

Or perhaps you prefer this link to the cppreference.com site. I actually do..

[shiftleftplusone]

as this description is written for beginners, the term "calculation error" should be precise enough. Providing too much information is puzzling IMO. Advanced C users are supposed to know about this issue (CMIIW) ^^ . I already edited this point in my description above.

[agdl]

@systronix unlucky we don't have a contributing platform for documentation yet so the only way to contribute at the moment is to open issues like this on git.

However following yours tips I wrote this sentence that is online now:

*Note:

Please note that the return value for millis() is a 32-bit unsigned long, calculation errors are generated if a programmer tries to do math with other smaller datatypes such as 16-bit int.*

Is it good for everybody?

[shiftleftplusone]

+1

[Chris--A]

@agdl

It doesn't really cover it I think.

I wrote the sample below to specifically point out that its not just smaller datatypes. And no errors are generated, there is only 'incorrect operation'. Sorry for being picky, but that sentence does not do it justice.

Note: the return value for millis() is an unsigned long, logic errors may occur if a programmer tries to do arithmetic with smaller data types such as int's. Even signed long may encounter errors as its maximum value is half that of its unsigned counterpart.

[agdl]

@Chris--A you are right. Now your rephrase is on-line!

[Chris--A]

Cool cool.

[agdl]

can we close this issue?

[Szepi]

While not perfect, it's ok with me. Cheers, Csaba

[ElectricRCAircraftGuy]

Good enough for me too.

[shiftleftplusone]

super-perfect +1

[Chris--A]

@agdl There is a typo error in the updated documentation:

Please not that the return....

[agdl]

@Chris--A done @systronix let me know if it is ok for you to close the issue!

[agdl]

I'm closing this. Reopen if further modifications are needed!