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 dtostrf() issue using trackerj/odometer fixes along with my own fix for string null character ending #664

Merged
merged 5 commits into from
Aug 13, 2015

Conversation

martinayotte
Copy link
Contributor

fix dtostrf() issue using trackerj/odometer fixes along with my own fix for string null character ending

@martinayotte
Copy link
Contributor Author

Hi Igor,
I must admit that is not my own code (as mentioned in the commit).
It comes from that thread http://forum.arduino.cc/index.php?topic=329949.msg2344748#msg2344748
My only contribution was the last line "*out = 0;" :-)
Although I made few tests, I've not covered all test cases ... :-(

@martinayotte
Copy link
Contributor Author

Hi Igor,
I took time to revised the dtostr() function, and yes it was indeed a bit ugly (sorry about not having done that myself before). I think you can take a second look since the PR now includes both commits.

@igrr
Copy link
Member

igrr commented Aug 12, 2015

What situation does this change help fix? I.e. what were the cases where the old version didn't work correctly?

@martinayotte
Copy link
Contributor Author

Hi Ivan,
Do you mean the last version or the whole thing ?
last version handle better the "minimum width" and get rid of the OVF code, it was useless since it is relying on a double not a uint.
If you mean the whole thing, the original file was buggy in several case, even in calculations. Here are example of output with the old and new versions :

sqrt(2.00)=1.41
1 .00000000002147483647
12.3457
-12.3457
2
ovf

newer version :

sqrt(2.00)=1.41
1.41421356237309510106
12.3457
-12.3457
2
5000000000.00000

EDIT : oupps ! it seems that github reformat the spaces and tabs ... I will send you this thru PM ...
EDIT2 : could not add an attachment either ... Now trying screenshot :
dtostrf-snapshot

@martinayotte
Copy link
Contributor Author

Hi Ivan,
Like I though and was scared of, GitHub simply swallowed any new commit in the same PR right after the commit on my repro. I can't associate commits to specific PR with some kind of "cherry-picking", you will have to handle it on your side.
(Maybe I miss some github learning, can multiple PRs can co-exist from single user ? Right now, I feel that GitHub only allows 1 PR per fork)

So, latest commit is not related to dtostrf(), but to SPIFFS, as I mentioned to you ealrier : it provide fileSize() in Dir object when used during openDir().

On related subject, did you got chance to investigate the 4096 fileSize limit ? should I open a bug for that ?
Also, any thing new about SdFat clashes ? maybe related to namescope ... Should I open a but for that too ?

Ciao !

@igrr
Copy link
Member

igrr commented Aug 13, 2015

On github, one branch is linked to one pull request, so if you want to open another one, just checkout the commit at which your local and remote branches diverge, and create a new branch. I'm merging all this now, everything looks fine.

Regarding the file size limit, i haven't yet had time to fix, but i can confirm that on ESP side files over 4k are created just fine. I will take a look at this a bit later, no need to open an issue.
Regarding SdFat, you are welcome to open an issue or even propose a solution — I'm not familiar with this library but probably the issue has something to do with the fact that both libraries define File classes :) So the solution would be either to

  • not use SdFat from the same file as FS
  • or, rename File and Dir classes to FSFile and FSDir.

Personally I would stick with the first but I can imagine some would be more comfortable with the second.
So you are welcome to open an issue, if even for comments and discussion.

igrr added a commit that referenced this pull request Aug 13, 2015
fix dtostrf() issue, add Dir::fileSize
@igrr igrr merged commit d075d8b into esp8266:esp8266 Aug 13, 2015
@martinayotte
Copy link
Contributor Author

H Ivan,
For the SdFat issue, I've tried your suggestion : I've relocated all my SdFat code in an SdFatHelper.cpp, exposing only the prototype of my functions in SdFatHelper.h.
The problem is that ArduinoIDE deciding himself about the include paths needed, according to libraries included in the main sketch. So, the SdFatHelper.cpp doesn't find the SdFat.h since it has not been included by the main sketch.
So, I'm bit of stucked, I don't have much choice to go with the second solution ...

igrr added a commit that referenced this pull request Oct 29, 2015
fix dtostrf() issue, add Dir::fileSize
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.

2 participants