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(yaml): properly escape double quotes #3965

Merged
merged 4 commits into from
Nov 12, 2023
Merged

Conversation

raphaelcoeffic
Copy link
Member

Summary of changes:

  • YAML output: escape double quotes " inside string elements

@philmoz
Copy link
Collaborator

philmoz commented Aug 26, 2023

Does Companion also need a similar fix?

@raphaelcoeffic
Copy link
Member Author

Does Companion also need a similar fix?

I must admit I didn’t try. Chances are that we don’t, as the YAML lib is probably already doing the right thing.

@philmoz
Copy link
Collaborator

philmoz commented Aug 26, 2023

I tried this PR on my TX16S. If I add a '"' to a model name on the radio it works.

However if I then load the radio/model settings into Companion and try and run the simulator it crashes.

Companion converts the \x22 string back to a '"' character - so the escaping is lost.

@philmoz
Copy link
Collaborator

philmoz commented Aug 26, 2023

Also Companion does not currently allow entering '"' characters into the model name.

If this is going to be allowed on the radio is should probably also be allowed in Companion.

@raphaelcoeffic
Copy link
Member Author

If this is going to be allowed on the radio is should probably also be allowed in Companion.

Absolutely. That means we need to dig into the YAML lib and find out how we can have proper escaping.

@pfeerick pfeerick linked an issue Oct 8, 2023 that may be closed by this pull request
1 task
@pfeerick pfeerick added this to the 2.10 milestone Oct 8, 2023
@pfeerick pfeerick added the bug 🪲 Something isn't working label Oct 8, 2023
@philmoz
Copy link
Collaborator

philmoz commented Oct 8, 2023

Brute force solution for Companion. Force ALL output to be double quoted (and I really mean ALL).
File - companion/src/firmwares/edgetx/edgetxinterface.cpp

static void writeYamlToByteArray(const YAML::Node& node, QByteArray& data, bool addChecksum = false)
{
    // TODO: use real streaming to avoid memory copies
    YAML::Emitter out;
    out.SetStringFormat(YAML::EMITTER_MANIP::DoubleQuoted);

    out << node;
    data = QByteArray::fromStdString(out.c_str());

    qDebug() << "Saving YAML:";

    if(addChecksum) {
      uint16_t checksum = 0xFFFF;
      checksum = calculateChecksum(data, checksum);
      std::stringstream checksum_ostream;
      checksum_ostream << "checksum:  " << checksum << std::endl;
      data.prepend(checksum_ostream.str().c_str());
    }

    qDebug() << data.toStdString().c_str();
}

@raphaelcoeffic
Copy link
Member Author

// TODO: use real streaming to avoid memory copies

Would make sense, right? There might be some implications with the ZIP compression as well.

@philmoz
Copy link
Collaborator

philmoz commented Nov 2, 2023

Instead of trying to hack the yaml-cpp output (which is technically correct), I've pushed a change so that the firmware can correctly read back the strings saved in Companion that contain embedded double quotes.

Also changed Companion to allow the '"' character in model names.

@raphaelcoeffic
Copy link
Member Author

@philmoz If I understand it correctly, your change would effect the following:

On the radio side:

  • strings are always emitted with double-quotes, thus leading to "My 5\" Quad".
  • when reading attribute values, double quotes would only be removed from the value when the first non-space character is a double-quote:
    • attribute: "my value" leads to a value of my value,
    • while attribute: my " value" would lead to my " value").

On the companion side:

  • string attributes are basically never double-quoted (any exceptions?)

@philmoz
Copy link
Collaborator

philmoz commented Nov 6, 2023

On the radio strings are always enclosed in double quotes.
An embedded double quote is output as \x22, e.g. "My 5\x22 Quad". This is the first change done in this PR.
The radio and Companion YAML readers already handle characters escaped like this.

On Companion strings are not enclosed in double quotes unless they contain special characters (as per the YAML spec).
Companion would output as My 5" Quad.

The latest change modifies the radio YAML reader to handle the case where a string is not enclosed in quotes; but contains an embedded quote.

@raphaelcoeffic
Copy link
Member Author

Thx @philmoz! Would you mind adding some unit test? 😅

@pfeerick pfeerick added needs: unit tests Unit tests are needed to for continuous validation companion Related to the companion software don't merge and removed needs: companion needs: unit tests Unit tests are needed to for continuous validation don't merge labels Nov 6, 2023
@philmoz philmoz force-pushed the escape-double-quotes branch from 7d419ae to 9b5339c Compare November 10, 2023 23:36
Copy link
Member

@pfeerick pfeerick left a comment

Choose a reason for hiding this comment

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

Working good on TX16S and TX12MK2, both on radio (not longer mangling model) and when syncing with Companion.

Two aspects that could be improved on B&W - you can't select double quote as a input character - I think this may be raised in an older issue regarding order and punctuation, but come to light again now. And secondly, while the quote is visible for smaller font, it's not visble in the larger font that displays the model name (on 128x64, at least).

@pfeerick pfeerick merged commit 0e7fe7f into main Nov 12, 2023
40 checks passed
@pfeerick pfeerick deleted the escape-double-quotes branch November 12, 2023 23:34
@dizzysizzy
Copy link

hey, i have the same issue, i want to name my model 5" quad but it just erases all the data on that model after a reboot (tx16s) and im on the newest version of edge tx, is there a way to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working companion Related to the companion software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Won't save model with " in the name
4 participants