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 Print::printf implementation #64

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Conversation

tsl0922
Copy link
Contributor

@tsl0922 tsl0922 commented Jan 10, 2025

Requirement: h2zero/platform-n-able#3.

Fixes #62.

@h2zero
Copy link
Owner

h2zero commented Jan 12, 2025

Thanks! Only concern I have is that it will likely cause a stack overflow for constrained devices, nRF51 and nRF52810 etc.. I will test it and see.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Jan 13, 2025

https://github.com/esp8266/Arduino/blob/bb79e9076e8402c81c24efbabd19f1baab1c6bd1/cores/esp8266/Print.cpp#L50-L71

The code above maybe better, it uses a smaller buffer first, alloc if the buffer overflow.

@h2zero h2zero mentioned this pull request Jan 14, 2025
@tsl0922 tsl0922 force-pushed the fix-printf branch 4 times, most recently from 3992712 to 4e203bb Compare January 15, 2025 02:30
@tsl0922
Copy link
Contributor Author

tsl0922 commented Jan 15, 2025

Follow up to #68 (comment)

I have a working implementation now, if you are OK with the changes, I can open another PR to replace this one @h2zero

--- a/builder/frameworks/arduino/nrf5.py
+++ b/builder/frameworks/arduino/nrf5.py
@@ -71,6 +71,8 @@ env.Append(

     CPPPATH=[
         join(FRAMEWORK_DIR, "cores", board.get("build.core")),
+        join(FRAMEWORK_DIR, "cores", board.get("build.core"),
+            "libc", "printf"),
         join(FRAMEWORK_DIR, "cores", board.get("build.core"),
              "nordic", "nrfx"),
         join(FRAMEWORK_DIR, "cores", board.get("build.core"),
--- a/cores/nRF5/Print.cpp
+++ b/cores/nRF5/Print.cpp
@@ -22,6 +22,7 @@
 #include <string.h>
 #include <math.h>
 #include "Arduino.h"
+#include "printf.h"

 #include "Print.h"

@@ -187,11 +188,16 @@ size_t Print::println(const Printable& x)
   return n;
 }

+extern "C" void streamOutput(char character, void* arg)
+{
+  (static_cast<Print *>(arg))->write(character);
+}
+
 int Print::printf(const char* format, ...)
 {
   va_list va;
   va_start(va, format);
-  int ret = vprintf(format, va);
+  int ret = vfctprintf(&streamOutput, this, format, va);
   va_end(va);
   return ret;
 }

@h2zero
Copy link
Owner

h2zero commented Jan 15, 2025

@tsl0922 That is probably the better solution, please open PR's for this.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Jan 15, 2025

@tsl0922 That is probably the better solution, please open PR's for this.

Done. This PR has been updated, PR for platform: h2zero/platform-n-able#3.

Copy link
Owner

@h2zero h2zero left a comment

Choose a reason for hiding this comment

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

Looks good, just need to fix a couple things.

cores/nRF5/Print.cpp Outdated Show resolved Hide resolved
cores/nRF5/Print.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@h2zero h2zero left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@h2zero h2zero merged commit c3d85f6 into h2zero:master Jan 15, 2025
62 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.

Print::printf implementation issue
2 participants