Next release v2.0.0. Clarify protocol. Disruptive changes: see below

[doegox]

This huge commit tries to enhance several things related to the fw/cli protocol. Generally, the idea is to be verbose, explicit and reuse conventions, in order to enhance code maintainability and understandability for the other contributors.

docs/protocol.md got heavily updated

Many commands have been renamed for consistency. you are invited to adapt your client for easier maintenance

Guidelines, also written in docs/protocol.md "New data payloads: guidelines for developers":

• Now protocol data exchanged over USB or BLE are defined in netdata.h as packed structs and values are stored in Network byte order (=Big Endian)
• Command-specific payloads are defined in their respective cmd_processor handler in app_cmd.c and chameleon_cmd.py
• Define C struct for cmd/resp data greater than a single byte, use and abuse of struct.pack/struct.unpack in Python. So one can understand the payload format at a simple glimpse.
• If single byte of data to return, still use a 1-byte data, not status.
• Use unambiguous types such as uint16_t, not int or enum. Cast explicitly int and enum to uint_t of proper size
• Use Network byte order for 16b and 32b integers
  • Macros U16NTOHS, U32NTOHL must be used on reception of a command payload.
  • Macros U16HTONS, U32HTONL must be used on creation of a response payload.
  • In Python, use the modifier ! with all struct.pack/struct.unpack
• Concentrate payload parsing in the handlers, avoid further parsing in their callers. This is true for the firmware and the client.
• In cmd_processor handlers: don't reuse input length/data parameters for creating the response content
• Avoid hardcoding offsets, use sizeof(), offsetof(struct, field) in C and struct.calcsize() in Python
• Use the exact same command and fields names in firmware and in client, use function names matching the command names for their handlers unless there is a very good reason not to do so. This helps grepping around. Names must start with a letter, not a number, because some languages require it (e.g. 14a_scan not possible in Python)
• Respect commands order in m_data_cmd_map, data_cmd.h and chameleon_cmd.py definitions
• Even if a command is not yet implemented in firmware or in client but a command number is allocated, add it to data_cmd.h and chameleon_cmd.py with some FIXME: to be implemented comment
• Validate data before using it, both when receiving command data in the firmware and when receiving response data in the client.
• Validate response status in client.

Disruptive changes:

• GET_DEVICE_CAPABILITIES: list of cmds in data are now really Big Endian Note: the initial attempt to use macros PP_HTONS were actually considering wrongly that the platform was Big Endian (BYTE_ORDER was actually undefined) while it is actually Little Endian.
• GET_APP_VERSION: response is now a tuple of bytes: major|minor (previously it was in reversed order as a single uint16_t in Little Endian)
• SET_SLOT_TAG_TYPE: tag_type now on 2 bytes, to use updated tag_specific_type_t enum
• SET_SLOT_DATA_DEFAULT: tag_type now on 2 bytes, to use updated tag_specific_type_t enum
• GET_SLOT_INFO: tag_type now on 2 bytes, to use updated tag_specific_type_t enum
• GET_DEVICE_CHIP_ID: now returns its 64b ID following Network byte order (previously, bytes were in the reverse order)
• GET_DEVICE_ADDRESS: now returns its 56b address following Network byte order (previously, bytes were in the reverse order). CLI does not reverse the response anymore so it displays the same value as before.
• MF1_GET_DETECTION_COUNT: now returns its 32b value following Network byte order (previously Little Endian)
• GET_GIT_VERSION response status is now STATUS_DEVICE_SUCCESS
• GET_DEVICE_MODEL response status is now STATUS_DEVICE_SUCCESS
• MF1_READ_EMU_BLOCK_DATA response status is now STATUS_DEVICE_SUCCESS
• GET_DEVICE_CAPABILITIES response status is now STATUS_DEVICE_SUCCESS
• HF14A_SCAN: entirely new response format, room for ATS and multiple tags
• MF1_DETECT_SUPPORT response status is HF_TAG_OK if MF1 else an error (HF_TAG_NO if the tag didn't reply to auth)
• MF1_DETECT_PRNG response status is now HF_TAG_OK and prng_type is returned in 1 byte of data with a new enum mf1_prng_type_t == MifareClassicPrngType
• MF1_STATIC_NESTED_ACQUIRE was added
• MF1_DETECT_DARKSIDE was removed
• MF1_DARKSIDE_ACQUIRE response status is now HF_TAG_OK and darkside_status is returned in 1 byte of data. If OK, followed by 32 bytes as previously
• MF1_GET_ANTI_COLL_DATA: in case slot does not contain anticoll data, instead of STATUS_PAR_ERR, now it returns STATUS_DEVICE_SUCCESS with empty data
• MF1_SET_ANTI_COLL_DATA and MF1_GET_ANTI_COLL_DATA now use the same data format as HF14A_SCAN
• GET_ENABLED_SLOTS: returns twice more bytes as now there is a enabled_hf and a enabled_lf
• SET_SLOT_ENABLE: requires sense_type to tell which of HL or HF should be enabled
• HF14A_RAW: new command
• DATA_CMD_GET_DEVICE_MODEL now matches chameleon_device_type_t
• MF1_GET_WRITE_MODE must handle new SHADOW_REQ indicating a move to SHADOW on the news data save

For clients to detect Ultra/Lite with older firmwares, one can issue the GET_APP_VERSION and urge the user to flash his device if needed. On older firmwares, it will return a status=b'\x00' and data=b'\x00\x01' while up-to-date firmwares will return status=STATUS_DEVICE_SUCCESS and data greater or equal to b'\x01\x00' (v1.0).

Other changes: cf CHANGELOG, and probably a few small changes I forgot about..

[github-actions[bot]]

You are welcome to add an entry to the CHANGELOG.md as well

[github-actions[bot]]

Built artifacts for commit 963d4d62f1bc6da5a079ab7c67317c2530d2af95

Firmware

• Ultra APP DFU Package
• Ultra binaries
• Lite APP DFU Package
• Lite binaries

Client

• Linux
• macOS
• Windows

[xianglin1998]

A very perfect idea. I estimate that I will only be available next week to continue contributing, as I am too busy this week.

[taichunmin]

Should tag_type in DATA_CMD_GET_SLOT_INFO cmd response to be change to 2 bytes?

[taichunmin]

Should we rename the GET/SET_BLE_CONNECT_KEY_CONFIG to GET/SET_BLE_PAIRING_KEY_CONFIG ?

Because the cmd GET/SET_BLE_PAIRING_ENABLE using the word PAIRING.

[doegox]

Should we rename the GET/SET_BLE_CONNECT_KEY_CONFIG to GET/SET_BLE_PAIRING_KEY_CONFIG ?

Because the cmd GET/SET_BLE_PAIRING_ENABLE using the word PAIRING.

yes, or just GET/SET_BLE_PAIRING_KEY

[taichunmin]

Should 4002: MF1_SET_ANTI_COLLISION_INFO to be detele?

Should 4001: MF1_SET_ANTI_COLLISION_RES to be rename to MF1_SET_ANTI_COLL_DATA because of 4018: MF1_GET_ANTI_COLL_DATA?

[taichunmin]

The request frame of MF1_SET_ANTI_COLLISION_RES is inconsistent with response frame of MF1_GET_ANTI_COLL_DATA. Should we modify one of them?

MF1_SET_ANTI_COLLISION_RES: SAK (1 Byte) + ATQA (2 Byte) + UID (4, 7 or 10 Bytes) MF1_GET_ANTI_COLL_DATA: UID (10 Bytes) + UID Size (2 Bytes) + SAK (1 Byte) + ATQA (2 Bytes) + Unknown (1 Byte)

[doegox]

The request frame of MF1_SET_ANTI_COLLISION_RES is inconsistent with response frame of MF1_GET_ANTI_COLL_DATA. Should we modify one of them?

MF1_SET_ANTI_COLLISION_RES: SAK (1 Byte) + ATQA (2 Byte) + UID (4, 7 or 10 Bytes) MF1_GET_ANTI_COLL_DATA: UID (10 Bytes) + UID Size (2 Bytes) + SAK (1 Byte) + ATQA (2 Bytes) + Unknown (1 Byte)

and inconsistent with HF14A_SCAN which I'd like to simplify as well :) and allow for more than 1 tag.

So sth like

• HF14A_SCAN: tag1|tag2|... with tagN = uidlen[1]|uid[uidlen]|atqa[2]|sak[1]|atslen[1]|ats[atslen] (atslen=0 if no ats)
• HF14A_GET_ANTI_COLL_DATA: cf tag above
• HF14A_SET_ANTI_COLL_DATA: cf tag above (and yes HF14A, this is not specific to MF1)

[taichunmin]

Also rename MF1_GET_BLOCK_ANTI_COLL_MODE and MF1_SET_BLOCK_ANTI_COLL_MODE to hf14a?

[doegox]

Also rename MF1_GET_BLOCK_ANTI_COLL_MODE and MF1_SET_BLOCK_ANTI_COLL_MODE to hf14a?

These ones are specific to tags storing their anticol data in a block, so only 4-byte MF1

[augustozanellato]

I wish I saw this before submitting my last PR lol, feel free to cherrypick stuff and merge it here

[taichunmin]

typo in PR description:

MF1_DARKSIDE_ACQUIRE response status is now HF_TAG_OK and darkside_status is returned in 1 byte of data. If OK, followed by 24 bytes as previously

It should be 32 bytes, not 24 bytes

[taichunmin]

The MF1_DARKSIDE_DETECT command in PR description is wrong. (Because of being removed)

[doegox]

Thank you @taichunmin ! I fixed both. In PR and in new commit.

[Foxushka]

MF1_DETECT_SUPPORT will return status 1 if card doesn't support Mifare Classic (not 0 in .data[0])

[doegox]

MF1_DETECT_SUPPORT will return status 1 if card doesn't support Mifare Classic (not 0 in .data[0])

ha yes indeed, data is redundant, I removed it, thanks

[doegox]

ok, so far, unless bugs, I think we're done with the protocol changes. The only non uniformity I see remaining is that some commands spread bits into bytes (much easier to parse in Python and probably not impacting much the comms) while some are using bitfields (mf1_get_detection_log because it's how it is stored in flash, and HF14A_RAW options). I don't have strong opinion on staying as such or moving in one or the other direction... Any comment ?

@GameTec-live @Foxushka @taichunmin @whywilson are you ready in your CLI/SDK for the merge of this PR?

BTW how could we make it easier ? It's hard to have to get all clients to be ready for the move exactly at the same moment. Having a way to indicate to your client to not upgrade the fw to the latest version unless the client got upgraded. Any suggestion how?

[taichunmin]

There are not many developers using the chameleon-ultra.js SDK yet, so this Pull Request has a small impact on my SDK.

[xianglin1998]

Can we start iterating versions and have the app/client choose whether to update the device firmware based on the last supported device firmware version?

[nemanjan00]

Looking at the API of github release creation, it does not seem to support any form of metadata, so I propose to use tags and semver.

x.y.z

x - breakage of compatibility of command y - new command z - bug fix

This way, they know that they need to take manual action on major (x)

[augustozanellato]

Looking at the API of github release creation, it does not seem to support any form of metadata, so I propose to use tags and semver.

semver implies releases, at the moment there haven’t been any releases except for the factory firmware. We’d probably have to start making periodic releases and tagging those with semver, while keeping the dev build as “unstable, you’re on your own, here be dragons”

[whywilson]

Can we start iterating versions and have the app/client choose whether to update the device firmware based on the last supported device firmware version?

Necessary. When users encounter problems, they can now only recommend updating to the latest firmware and app, but cannot directly locate firmware problems.

MTools Lite can have fast updates to the changes. It's time to implement.

[doegox]

Looking at the API of github release creation, it does not seem to support any form of metadata, so I propose to use tags and semver.

x.y.z

x - breakage of compatibility of command y - new command z - bug fix

This way, they know that they need to take manual action on major (x)

FW_VER_NUM of nRF SDK is only x.y (advertised over BLE & USB) and the fw can also provide its GIT_VERSION as per git describe --abbrev=7 --dirty --always --tags That's for the code itself.

Then for git releases we can use vx.y.z annotated tags, just make sure to have the same x and y as above...

I propose to:

• give a couple of days for clients to implement logic to only accept releases v1.x and commits above (but not commits >= new tag v2.x.x)
• merge PR and tag it as v2.0.0 with a FW_VER_NUM=2.0
• clients can then be upgraded to accept v2.x.x releases with their breaking changes, and possibly with 2 channels:
  • the release channel: only releases tagged v2.x.x
  • the dev channel and its automatic builts (from the main branch) for all commits (above v2.x.x) until the next v3.0.0 tag

[doegox]

I updated the proposal to avoid bumping versions to 2.0.0 and 3.0.0 one after the other

[nemanjan00]

Maybe we can take version from latest git tag instead?

TAG=$(git describe --tags --abbrev=0 --match "v*.*") # match tag that looks like version

MAJOR=$(echo $TAG | cut -d "." -f1) # take first segment of tag
MAJOR=${MAJOR:1} # remove 'v'

MINOR=$(echo $TAG | cut -d "." -f2) # take second segment

echo $MAJOR.$MINOR

Something like this?

[doegox]

Hmm we may try but we need fallbacks for when compiling not from a repo. (source zip, source in distros (one day... :) )

For ref, here is how we do for Proxmark in Makefile and mkversion.sh

Knowing that here we would go for semver and we have automatic releases so the workflow creating releases must make sure the produced release source can compile and show decent version info.

[Foxushka]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one)

@whywilson your app ready?

[GameTec-live]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one)

@whywilson your app ready?

nope, sadly not... still some major bugs...

[Foxushka]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one) @whywilson your app ready?

nope, sadly not... still some major bugs...

Not critical, anyway firmware should be merged firstly

[doegox]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one) @whywilson your app ready?

nope, sadly not... still some major bugs...

Not critical, anyway firmware should be merged firstly

does MTools allow for fw upgrades too? If yes is it also automatic from latest commit? Else can we merge ?

[doegox]

@nemanjan00 I pushed mechanics in Makefile to get APP_FW major & minor. This does not solve yet the issue in source releases but one step after the other one :) Update: oops it breaks the CI...

[whywilson]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one)

@whywilson your app ready?

nope, sadly not... still some major bugs...

Not critical, anyway firmware should be merged firstly

does MTools allow for fw upgrades too? If yes is it also automatic from latest commit? Else can we merge ?

No. MTools Lite works on Bluetooth. After merging the code, will the version number of fw be iterated normally? Or only determine the firmware version by git id for the moment?

[doegox]

From our side everything is ready, waiting for merge into main (as our app WILL require new protocol and won't work with earlier one)

@whywilson your app ready?

nope, sadly not... still some major bugs...

Not critical, anyway firmware should be merged firstly

does MTools allow for fw upgrades too? If yes is it also automatic from latest commit? Else can we merge ?

No. MTools Lite works on Bluetooth. After merging the code, will the version number of fw be iterated normally? Or only determine the firmware version by git id for the moment?

I'm trying to base the fw version x.y automatically on the latest existing git tag "vx.y.z" , which will be created manually at least every time the protocol changes (x+1 if breaking, y+1 if new command) but so far it fails in the CI... Despite fetch-depth: 0 it fetches with --depth=1 cf https://github.com/RfidResearchGroup/ChameleonUltra/actions/runs/6305449165/job/17118902995?pr=147#step:2:52 @augustozanellato any idea why and how to fix it ?

[nemanjan00]

I am not a GitHub CI guy (I prefer GitLab), but, log

Job defined at: RfidResearchGroup/ChameleonUltra/.github/workflows/build_firmware.yml@refs/heads/main

makes it sound like you need to change script in main, it does not seem to want to use scripts from PR, probably for security reasons

[doegox]

I am not a GitHub CI guy (I prefer GitLab), but, log

Job defined at: RfidResearchGroup/ChameleonUltra/.github/workflows/build_firmware.yml@refs/heads/main

makes it sound like you need to change script in main, it does not seem to want to use scripts from PR, probably for security reasons

Strange, if I do a new branch from the PR, then it works fine https://github.com/RfidResearchGroup/ChameleonUltra/actions/runs/6305741100/job/17119709062 Uses: RfidResearchGroup/ChameleonUltra/.github/workflows/build_firmware.yml@refs/heads/test_again_ci

[nemanjan00]

My experience with GH CI is very limited, but I assume it has something to do with branch being PR

[doegox]

Thanks, it should work once merged :) Release v2.0.0 is ready to be pushed.

[nemanjan00]

I think it is due to pull_request_target

https://github.com/orgs/community/discussions/27084#discussioncomment-3254544

[doegox]

Release ready, preview available at https://github.com/doegox/ChameleonUltra/releases/tag/v2.0.0 @Foxushka @GameTec-live tell me when I can push it to the repo.

[augustozanellato]

I think it is due to pull_request_target

https://github.com/orgs/community/discussions/27084#discussioncomment-3254544

It indeed is. It was documented on the refactored ci PR. Tl;dr pull_request_target takes the workflows files from the pr target branch, so you can’t test CI changes before merging them. pull_request_target is required in order to have a token with write access that is used to push the builder docker image on PRs.

[doegox]

I think it is due to pull_request_target https://github.com/orgs/community/discussions/27084#discussioncomment-3254544

It indeed is. It was documented on the refactored ci PR. Tl;dr pull_request_target takes the workflows files from the pr target branch, so you can’t test CI changes before merging them. pull_request_target is required in order to have a token with write access that is used to push the builder docker image on PRs.

thanks for the explanation @augustozanellato ! I added a section in the yaml to build releases on vx.y.z tags as well : https://github.com/RfidResearchGroup/ChameleonUltra/pull/147/commits/099fb6b912eaf947fed736c7ed8970972c3308c5 tested on my fork : https://github.com/doegox/ChameleonUltra/releases/tag/v2.0.0 This looks ok for you ?

[augustozanellato]

I think it is due to pull_request_target https://github.com/orgs/community/discussions/27084#discussioncomment-3254544

It indeed is. It was documented on the refactored ci PR. Tl;dr pull_request_target takes the workflows files from the pr target branch, so you can’t test CI changes before merging them. pull_request_target is required in order to have a token with write access that is used to push the builder docker image on PRs.

thanks for the explanation @augustozanellato ! I added a section in the yaml to build releases on vx.y.z tags as well : 099fb6b tested on my fork : https://github.com/doegox/ChameleonUltra/releases/tag/v2.0.0 This looks ok for you ?

LGTM!

[doegox]

Apparently the GUI still needs some functional fixes so we'll wait a bit

[Foxushka]

What I skipped, why now git hash is v2.0.0? I've downloaded from https://github.com/doegox/ChameleonUltra/releases/tag/v2.0.0

[doegox]

What I skipped, why now git hash is v2.0.0? I've downloaded from https://github.com/doegox/ChameleonUltra/releases/tag/v2.0.0

Nothing changed, the git version string is the output of git describe --abbrev=7 --dirty --always --tags

$ git checkout main
Already on 'main'
Your branch is ahead of 'origin/main' by 40 commits.
  (use "git push" to publish your local commits)

$ git describe --abbrev=7 --dirty --always --tags
v2.0.0

$ echo foo > CHANGELOG.md 
$ git commit -a -m foo
[main 617d6d0] foo
 1 file changed, 1 insertion(+), 100 deletions(-)

$ git describe --abbrev=7 --dirty --always --tags
v2.0.0-1-g617d6d0

$ echo bar > CHANGELOG.md 

$ git describe --abbrev=7 --dirty --always --tags
v2.0.0-1-g617d6d0-dirty

[Foxushka]

So it won't cause issues later? Only one time bug?

[doegox]

So it won't cause issues later? Only one time bug?

Well I don't know what you are doing with the git version string in your app, but every time there will be a release from a release tag, the version string will just be the tag name...

[Foxushka]

We parse commit from last release (or action as fallback) and if it doesn't starts with reported version from chameleon we preform update

[doegox]

We parse commit from last release (or action as fallback) and if it doesn't starts with reported version from chameleon we preform update

when firmware git_version returns only a tag and no commit (e.g. on a release tag), you can get the corresponding hash with an API query, e.g.

wget -q -O - "https://api.github.com/repos/doegox/ChameleonUltra/tags"|jq '.[] | select(.name == "v2.0.0") | .commit | .sha'
"4747d3884d21e0df8549e3029a920ea390e0b00a"