Addition of encoder module with bas64 and hex encode/decode as per #925

[TerryE]

See Issue #925 for discussions on this.

[pjsg]

This taught me that the dev environment runs with chars being unsigned by default, Interesting...

[TerryE]

Yes, they are, but where the arithmetic goes wrong if they happen to be signed (e.g. x>>4) I specify the unsignedness explicitly. Still this works as we discussed the legacy crypto.toHex and the crypto.toBase64 functions work with and without the encoder module linked in.

[pjsg]

Your base64 code relies on the unsignedness of the char (and also on the fact that CHAR_MIN == 0)

[ABonner]

Did you mean to call this call_endcoder with a d, or just call_encoder?

[TerryE]

Yup you are right. Wonder why I didn't get a compile warning?

[ABonner]

Looks like you autocompleted all other references the same :)

[TerryE]

Your base64 code relies on the unsignedness of the char

Philip, two of the routines are essentially copied from crypto and make these assumptions, but you are right -- perhaps it is better just to use uint8_t * rather than char *. I'll give it a couple of more days for any more feedback and than do an updated PR to sweep all these comments up.

[TerryE]

@jmattsson Johny, have you any comments before I do this rework. I'd like to sweep all feedback into this, rather have to do another circle around.

[ABonner]

Should be "crypto.hash"

[jmattsson]

I haven't got time to investigate for myself right now, but is there any chance of re-using the ROM functions for base64 instead of needing our own? I think it'll depend on how/if they allocate memory.

PROVIDE ( base64_decode = 0x40009648 );
PROVIDE ( base64_encode = 0x400094fc );

[TerryE]

Good suggestion. I'll take a look. If easy to do, I'll also add this to the crypto.

[TerryE]

They use the ROM mem_malloc / mem_free rather than the SDK's RTOS compatible pvPortMalloc a.k.a. os_malloc that is used in the app. I assume that these aren't mix and match.

[jmattsson]

I seem to recall having had things crash & burn (literally and figuratively, correspondingly) when trying to do said mix & match, so no, I'd recommend against it. Unless you want to copy the base64 routines from rom into iram dynamically and hot-patch the alloc/free calls to point to something more compatible... ;)

No further comments from me then, ping me when you've done the next round here and I can merge.

[TerryE]

Unless you want to copy the base64 routines from rom into iram dynamically and hot-patch the alloc/free calls to point to something more compatible

Not worth it. The generated code saving just isn't worth this complexity. If we want to save .text space then there are lower hanging fruit to go after.

[jmattsson]

I really wasn't serious with that suggestion, Terry! With the amount of finicky stuff one would have to get right (like, where do I even load this to in iram without clobbering anything?) would with near certainty result in an increase of code size rather than a decrease. I'll take the maintainability of regular code over meta-code, thank you!

[TerryE]

Sorry Johny, I picked up a flea from some friends dog and woke up in the middle of the night with my leg in the middle of being a lunch meal for the flea and its kids and itching like hell -- so a shower and the guest bedroom later, I am now clearing off issues whilst the itching subsides. My brain isn't working too well!

[TerryE]

I've updated the files to echo the review comments. I've also fix a bug with the end packet base64 decode that my test vectors threw up. Perhaps one of the committers could merge this. 😄

[jmattsson]

Thanks Terry!