Blockstream / Jade

Jade hardware wallet
MIT License
315 stars 47 forks source link

Add M5StickCPlus2 Compatibility #141

Closed 3rdIteration closed 1 month ago

3rdIteration commented 3 months ago

Add Configuration Profile Add Code to Support Power Management for M5StickCPlus2

Does change power.c a bit, so worth double checking.

JamieDriver commented 3 months ago

This is amazing - many thanks.
We're looking at adding support for a few other devices atm and were discussing separating 'power.c' into multiple files, to be #def'd in as required by the hw - ie rather than having all the #ifdef'ing within the one file which is getting increasingly messy and confusing.

This PR might have to get put on hold for a bit, just because there's a lot of churn going on atm - and we (well, I) don't have one of these devices atm to test with - but will definitely circle back to it. Much appreciated, as always.

thisIsNotTheFoxUrLookingFor commented 3 months ago

It is not letting me add a custom blind pin oracle server with this firmware on my M5StickC Plus2

I can reset the oracle to defaults, I can do most the tasks but if I try set my server it reports:

"Received msg: {'id': '288738', 'error': {'code': -32602, 'message': 'Empty or invalid first URL'}}"

However I can see it is sending the correct JSON of:

"Sending: {'method': 'update_pinserver', 'id': '288738', 'params': {'urlA': '', 'urlB': None, 'pubkey': '023647fb92427013de57584facce3334d19775cb10ce026ba920079dcc3afbf778'}} as cbor of size 115"

command I run is: python3 set_jade_pinserver.py --serialport /dev/ttyACM0 --set-pubkey ~/server_public_key.pub --set-url <ip:port redacted>

thisIsNotTheFoxUrLookingFor commented 3 months ago

Screenshot 2024-06-19 053931

thisIsNotTheFoxUrLookingFor commented 3 months ago

If I just try set the URL alone it fails same error, even if I set URL and URL2 same error. Seems to be an issue parsing the URL it thinks it's empty when it is not

Update

It allows me to use a url like http://1.1.1.1:8080

Following the instructions at https://help.blockstream.com/hc/en-us/articles/12800132096793-Set-up-a-personal-blind-oracle I was entering ip:port as per the instructions but this fails. Seems this is parsing a http/https url now so need to specify the protocol?

I'm guessing at some point https was added and now we need to specify the protocol but the instructions have not been updated to match?

3rdIteration commented 3 months ago

This is amazing - many thanks. We're looking at adding support for a few other devices atm and were discussing separating 'power.c' into multiple files, to be #def'd in as required by the hw - ie rather than having all the #ifdef'ing within the one file which is getting increasingly messy and confusing.

This PR might have to get put on hold for a bit, just because there's a lot of churn going on atm - and we (well, I) don't have one of these devices atm to test with - but will definitely circle back to it. Much appreciated, as always.

No worries. :)

JamieDriver commented 3 months ago

@tortxoFFoxtrot yy url must begin 'http://' or 'https://'.

Thanks for pointing out the help page is wrong (or out of date) - I'll raise it with the relevant folks and get it updated.

I trust you got it working in the end ?

thisIsNotTheFoxUrLookingFor commented 3 months ago

@tortxoFFoxtrot yy url must begin 'http://' or 'https://'.

Thanks for pointing out the help page is wrong (or out of date) - I'll raise it with the relevant folks and get it updated.

I trust you got it working in the end ?

I did thanks yah.

JamieDriver commented 3 months ago

https://help.blockstream.com/hc/en-us/articles/12800132096793-Set-up-a-personal-blind-oracle updated, thanks. :+1:

valerio-vaccaro commented 2 months ago

photo_2024-07-12_21-38-23

Tested on my m5stickcplus2

thisIsNotTheFoxUrLookingFor commented 1 month ago

It works over bluetooth to green wallt app, but on PC over USB it just shows the Jade and ID number but everything is blank, doesn't ask for PIN or anything

image

And this is the error electrum gives image

thisIsNotTheFoxUrLookingFor commented 1 month ago

Does not work over USB. Cannot get it to work with either Green or Electrum. Seems to work over bluetooth, I mean it connects and creates a wallet in green in iOS, not sure if I trust it enough to fund it though, seems flaky as.

thisIsNotTheFoxUrLookingFor commented 1 month ago

Sparrow seems to give the same error as Electrum image

JamieDriver commented 1 month ago

Here: https://github.com/Blockstream/Jade/pull/141#issuecomment-2176832718

The 'JADE_VERSION' string does not seem right - this is probably what is confusing apps ...

3rdIteration commented 1 month ago

Sparrow seems to give the same error as Electrum image

This is just user error.

You forked the repository and then built it, without a tagging a release. Just build from my repo and it will tag the version properly and everything will work. (If you scroll up a bit and see a photo of a decide running it, you will notice that the firmware version is displaying in the bottom right, whereas your will just hand a hit commit hash)

thisIsNotTheFoxUrLookingFor commented 1 month ago

Sparrow seems to give the same error as Electrum image

This is just user error.

You forked the repository and then built it, without a tagging a release. Just build from my repo and it will tag the version properly and everything will work. (If you scroll up a bit and see a photo of a decide running it, you will notice that the firmware version is displaying in the bottom right, whereas your will just hand a hit commit hash)

Ah yes I see, yea I just checked out the Stick Plus 2 branch I didn't set a tag. Should I just got for v1.47?

Ah I see 1.30-cryptoguide is the latest

thisIsNotTheFoxUrLookingFor commented 1 month ago

Yea that's the business thanks

image

3rdIteration commented 1 month ago

Yea that's the business thanks

image

Good job sorting it out. :)

JamieDriver commented 1 month ago

Hi @3rdIteration, We have finally merged a large branch adding (still early days / experimental, at this point ...) esp32s3 support. This branch changed the Kconfig file a bit, and split the power.c file, which was getting a bit messy/unwieldy.

I have rebased your work for this PR and updated it as I hope is appropriate. I have pushed my version here: https://github.com/Blockstream/Jade/tree/m5stickcplus2 If you are happy with that branch I'll squash my additional commits into my 'rebased' version of your commit, and merge it. Full props to you!
Ofc if you'd rather rework it yourself here that's also cool - whichever you prefer.

And many thanks @tortxoFFoxtrot for testing this stuff out - again if you want to validate my rebased version that would be much appreciated.

3rdIteration commented 1 month ago

Thanks and great to hear that there is experimental support for esp32-s3, I'll check it out.

Thanks for fixing up the stuff for Plus2, I have don't need to reinvent the wheel, so happy to go with what you have done, good job :)

JamieDriver commented 1 month ago

Thanks for fixing up the stuff for Plus2

I felt bad 'invalidating' all your work ;-) I just wanted to check that it all worked ok for you and I hadn't screwed it up ...

thisIsNotTheFoxUrLookingFor commented 1 month ago

And many thanks @tortxoFFoxtrot for testing this stuff out - again if you want to validate my rebased version that would be much appreciated.

Yes ok I will reflash with the new version and restore from seed and report back

thisIsNotTheFoxUrLookingFor commented 1 month ago

IMG_1643

JamieDriver commented 1 month ago

Rebased version merged to master @d48d60f3.

Much appreciated @3rdIteration and @tortxoFFoxtrot :pray:

Closing PR.

JamieDriver commented 1 month ago

Ah I see 1.30-cryptoguide is the latest

Did you mean 1.0.30-<something> ?

Just fyi: Jade uses 'semantic versioning', so 1.0.30-<something> will look like it's marginally "less than" 1.0.30 (as eg. 1.0.30-beta1 is "less than" 1.0.30). This is probably not an issue in most cases. If it is, maybe go for the next number up, '-<something>' ...

It is def a good idea to use Jade's latest tag's main 3 numbers if possible, as some apps will be using these as a version check to know if certain functionality is available or enforce a minimum supported fw version, etc.

Maybe I need to add something to the docs somewhere ... :thinking: