Remove JSON <--> String conversions when reading/writing files

[chrisib]

Two main changes:

1. Refactor JSON file I/O to operate on file objects directly instead of loading data into a string in RAM and operating on that
2. When reading non-JSON files, return bytes instead of str if the mode is binary & there's an error opening the file

[awonak]

The original save state design allowed script authors to choose the serialization format of their choice, but I don't think anyone else was interested in using struct. Saving state as a dict without knowledge of the storage format really just makes more sense. What do you think about refactoring this further to abstract away the underlying format from the API , e.g. replace save_state_json(data) with just save_state(data) which still just expects a dict. That should simplify the API and docs for script authors. You could even replace JSON with YAML ;).

If this seems too out of scope, we can create an issue for the backlog too.

[chrisib]

I think there's maybe some merit in allowing the bytes storage. It could be useful for more efficient storage of sequences, since JSON tends to have a lot of bloat. But the string storage could probably get dumped, since a string could be saved as a JSON object anyway (though with extra quotation marks).

e.g.

>>> import json
>>> json.dumps("hello world")
'"hello world"'

Looking through the code, there's nowhere that actually uses the load|save_state_str outside of the tests, so I think removing it now would probably be easiest.

I lied; the bootloader currently uses load_state_str and save_state_str for saving the last-run script. Removing that now would introduce a one-time bug where after updating users would have to select their script from the menu again. Are we okay with this, or should we keep the string support around for that one use case?

[roryjamesallen]

the bootloader currently uses load_state_str and save_state_str for saving the last-run script. Removing that now would introduce a one-time bug where after updating users would have to select their script from the menu again

I've never concentrated on what happens when you update the firmware but I would have assumed it made you choose from the mnu again anyway, and it's so easy to choose an item from the menu that I don't think this would be a huge problem. People usually update the firmware to get new scripts so would likely be returning to the menu by choice anyway

[chrisib]

If we're aware of the (very slight) regression in the bootloader then I'm okay with that too. I've removed the load_state_str and save_state_str functions from europi_script. I've left the _bytes versions because that feels like something potentially more useful for future scripts that need to save raw binary data. We could use JSON + base64 encoding for that, but being able to store raw 0's and 1's feels like a good feature to have.

[mjaskula]

These changes look good to me. I'm a little embarrassed of my original implementation now. This is a good improvement.

I agree that the effect on the bootloader is fine. As long as the user isn't getting an error, and the only result is that they get dumped back in to the menu, then I don't see any harm there.