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 encoding start and end addresses config data command #9

Merged
merged 6 commits into from
Mar 1, 2023

Conversation

microbit-carlos
Copy link
Collaborator

PR for the current proposal.

@microbit-carlos microbit-carlos requested a review from finneyj April 22, 2021 18:07
@finneyj
Copy link

finneyj commented Apr 22, 2021

Thanks @microbit-carlos.

Quick query: why BASE64 rather than just 2 HEX digits? I know BASE64 is more efficient, but I though mapping each byte to two HEX digits would be simpler and more deterministic to compute in DAPLink?

i.e. the output data will always be twice as long as the input data... so it's easy to calculate offsets etc when a BLOCK READ request arrives from the USB host?

@microbit-carlos
Copy link
Collaborator Author

Yeah, the encoding is still open, I just wrote something down to get the PR up (and one consideration for base64 is the hope that it can be decoded by some browser supported API, to reduce the HTML/JS size). There are more details in Jonny's email if you haven't seen it yet.

@microbit-carlos microbit-carlos requested a review from gerargz April 23, 2021 08:18
@gerargz
Copy link
Collaborator

gerargz commented May 20, 2021

Hi @microbit-carlos , I forgot about this repo and instead updated the spec in microbit-foundation/microbit-design@05a25eb. Can you review the changes? Let me know if you need me to raise a PR here with those changes.

spec/index.md Outdated Show resolved Hide resolved
spec/index.md Outdated Show resolved Hide resolved
@finneyj
Copy link

finneyj commented May 20, 2021

Looks fine to me other than checking the use of "base64" in the docs.

This will be enough to enable binary file system visualization.

Query though: should we also have an end address for this encoding? So specify a "window" which to perform the encoding, rather than assume that the encoding should be done until the end of the file?

I don't think we need it right now, but might be useful for future proofing? What do you think @microbit-carlos @gerargz ?

spec/index.md Outdated Show resolved Hide resolved
@microbit-carlos microbit-carlos changed the title Add Base64 encoding start address config data command Add encoding start and end addresses config data command May 20, 2021
@microbit-carlos
Copy link
Collaborator Author

Thanks everyone, I've update the the PRs with the comments.
@finneyj @gerargz could you have another look to ensure everything is correct?

@finneyj
Copy link

finneyj commented May 20, 2021

looks good to me!

@gerargz
Copy link
Collaborator

gerargz commented May 20, 2021

Looks good to me as well! 👍

@microbit-carlos
Copy link
Collaborator Author

Awesome, thanks everyone! I'll keep this open in case any tweaks are needed after implementation on the CODAL side.

@finneyj
Copy link

finneyj commented May 20, 2021

@gerargz and I don't have bugs. We just have unique behaviour that people haven't discovered yet. ;)

@microbit-carlos microbit-carlos changed the base branch from main to v2-branch August 13, 2021 18:45
@microbit-carlos microbit-carlos merged commit 00b5ba2 into v2-branch Mar 1, 2023
@microbit-carlos microbit-carlos deleted the base64 branch March 1, 2023 13:17
microbit-carlos added a commit that referenced this pull request Mar 1, 2023
* Add Base64 encoding start address config data command

* Update the encoding window storage command to have 2 pointers

* Update storage memory layout with the encoding window pointers

* Add storage Set encoding window default values

* Add additional note about remount needed for some storage commands.

* Add Set encoding window to the changelog.
@microbit-carlos microbit-carlos mentioned this pull request Mar 1, 2023
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.

3 participants