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

Add JsonVariant::printTo(char[N]) #2136

Open
zackees opened this issue Oct 17, 2024 · 7 comments
Open

Add JsonVariant::printTo(char[N]) #2136

zackees opened this issue Oct 17, 2024 · 7 comments
Labels
v7 ArduinoJson 7 wontfix

Comments

@zackees
Copy link

zackees commented Oct 17, 2024

This is with arduino json 7.2

inline void
jsUiManager::executeUiUpdates(const ArduinoJson::JsonDocument &doc) {
    auto& self = instance();
    for (ArduinoJson::JsonPairConst kv :
         doc.as<ArduinoJson::JsonObjectConst>()) {
        int id = atoi(kv.key().c_str());
        const char* value = kv.value().as<const char*>();   // <------ nullptr

        // double loop to avoid copying the string
        for (auto it = self.mComponents.begin(); it != self.mComponents.end();) {
            if (auto component = it->lock()) {
                ++it;
                if (component->id() == id) {
                    component->update(value);
                }
            } else {
                it = self.mComponents.erase(it);
            }
        }
    }
}
inline void
jsUiManager::executeUiUpdates(const ArduinoJson::JsonDocument &doc) {
    auto& self = instance();
    for (ArduinoJson::JsonPairConst kv :
         doc.as<ArduinoJson::JsonObjectConst>()) {
        int id = atoi(kv.key().c_str());
        std::string valueStr = kv.value().as<std::string>();  // <------ has expected value
        const char *value = valueStr.c_str();

        // double loop to avoid copying the string
        for (auto it = self.mComponents.begin(); it != self.mComponents.end();) {
            if (auto component = it->lock()) {
                ++it;
                if (component->id() == id) {
                    component->update(value);
                }
            } else {
                it = self.mComponents.erase(it);
            }
        }
    }
}

I would like to NOT have to create a bunch of memory strings in a tight loop. I'd like to just have JsonPairConst::value().printTo(charBuff);

@zackees zackees added the bug label Oct 17, 2024
@zackees
Copy link
Author

zackees commented Oct 17, 2024

This also fails to compile:

inline void
jsUiManager::executeUiUpdates(const ArduinoJson::JsonDocument &doc) {
    auto& self = instance();
    typedef char CharBuff[1024];
    CharBuff value = {0};
    for (ArduinoJson::JsonPairConst kv :
         doc.as<ArduinoJson::JsonObjectConst>()) {
        int id = atoi(kv.key().c_str());
        value = kv.value().as<CharBuff>();
        // double loop to avoid copying the string
        for (auto it = self.mComponents.begin(); it != self.mComponents.end();) {
            if (auto component = it->lock()) {
                ++it;
                if (component->id() == id) {
                    component->update(value);
                }
            } else {
                it = self.mComponents.erase(it);
            }
        }
    }
}

You should at least make as<const char*> static_assert so we don't get a nullptr exception during runtime.

@bblanchon bblanchon added question v7 ArduinoJson 7 and removed bug labels Oct 18, 2024
@bblanchon
Copy link
Owner

Hi @zackees,

kv.value() is a regular JsonVariant (or JsonVariantConst), so you can call serializeJson() as usual:

char buffer[1024];
serializeJson(kv.value(), buffer);

I cannot exclude as<const char*>() with a static assert because there are many legitimate uses.

Please read the documentation.

Best regards,
Benoit

@zackees
Copy link
Author

zackees commented Oct 18, 2024

It would be nice to have a comment in the source code. I dont' see why serializing to a static sized char buffer is an issue.

@bblanchon
Copy link
Owner

I don't think something like as<char[64]>() is possible because the array will decay to a pointer if it's used as a return type.

@zackees
Copy link
Author

zackees commented Oct 18, 2024

I don't think something like as<char[64]>() is possible because the array will decay to a pointer if it's used as a return type.

Well then why not just inline a printTo function right into the JsonVariant?

This would be obvious.

Right now the solution is "read the documentation"

which is claimed to be 700 pages long.

This isn't even a corner case. Reading values out of a document in a tight loop is like the most mainstream use case imaginable.

This seems like a one liner. Do I need to fork and submit a PR to move this forward?

Users around the world would rejoice.

@bblanchon
Copy link
Owner

Adding printTo() with all the legitimate overloads to JsonArray, JsonArrayConst, JsonDocument, JsonObject, JsonObjectConst, JsonVariant, and JsonVariantConst, is not exactly a one-liner.
Besides, it would add more pages to the documentation 😉

What's wrong with calling serializeJson()?

@zackees
Copy link
Author

zackees commented Oct 20, 2024

Just do

'''
template<int N>
void printTo(char[N] out);
'''

You don't have to support all overloads. Just give us the common path.

@bblanchon bblanchon changed the title JsonPairConst::value().as<const char*> returns null Add JsonVariant::printTo(char[N]) Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v7 ArduinoJson 7 wontfix
Projects
None yet
Development

No branches or pull requests

2 participants