sdcard_sd: Decode commands always as command

[harper23]

The SD card decoder predicts always the next token type from the current token type. This is working if the SD card is responding as expected. But if the card is not responding, the prediction is wrong. A second command token is decoded as a response token.

See example trace file. SD-card-init-001.zip

To fix this behavior the decoder state is set to St.GET_COMMAND_TOKEN, if the current token is a command token.

[sharkcz]

• please add the description to the commit message, the project is not using MRs
• the commit subject should be "sdcard_sd: Decode commands always as command" to follow the style and fix a typo

[harper23]

Am 08.12.2025 um 19:31 schrieb Dan Horák:

sharkcz left a comment (sigrokproject/libsigrokdecode#127) - please add the description to the commit message, the project is not using MRs - the commit subject should be "sdcard_sd: Decode commands always as command" to follow the style and fix a typo

Hi Dan, many thanks for your mail. It helps me to provide a pull request acceptable for the sigrok project. I am rather new to GitHub contribution. So, please allow me to describe, how I want to proceed. I will make a new commit using the --amend option to rewrite the commit message like this sdcard_sd: Decode commands always as command The SD card decoder predicts always the next token type from the current token type. This is working if the SD card is responding as expected. But if the card is not responding, the prediction is wrong. A second command token is decoded as a response token. To fix this behavior the decoder state is set to St.GET_COMMAND_TOKEN, if the current token is a command token. Then I push with the option --force-with-lease. This fixes the commit message, the pull request is still at the HEAD of my repository. I checked with another (private) repository how this change can be done. But the pull request title has still the typo. I hope this is okay. How do you think? Regards, Helge

[sharkcz]

Hi Helge,
yup, commit with --amend should allow you to update the message, then "git push --force" to update it in the PR. I believe you should be able to edit the PR title on the PR's web page.

[harper23]

Push is done. I hope this contribution is helpful and can be integrated in the sigrok repo.

[sharkcz]

Thanks, it looks good to me. Now we need someone familiar with the decoders who can do a review from a functional point of view.

[Krakonos]

@harper23 I didn't do a whole lot of checking, but I don't like the approach taken. The issue is that self.state is manipulated from within the get_token_bits helper function. This is unexpected behavior from a helper function. Moreover, I don't understand how this should possibly work, since the bit is set while in the function handle_response_r1, which is already too late. I think this works on the sample you submitted, because there are no regular packets. I'll check later on the data from sigrok-dumps repo to confirm.

However, even if I'm right I would suggest to change the implementation. This is a very unexpected sideeffect. My suggestion would be to read the first two bits in decode function and either reset the state if we received host transmission while waiting for a device response. All the parse functions would either have to be passed those bits or they could be removed, since they are set for each packet (correct me if I'm wrong, my SD protocol understanding is limited, but I believe first bit is always 0 (START), second bit is 1 for HOST transmission and 0 for DEVICE transmission.

[harper23]

@Krakonos This works because the implementation of all handle_cmd and handle_rsponse functions have a side effect. They actually not only handle some data but use the get_token_bits function to retrieve the bits. As long as not sufficient bits are read, these functions return. That's why this is not true:

since the bit is set while in the function handle_response_r1, which is already too late.

The number of expected bits are hard coded in the functions. The R2 response has 136 bits, the other 48. In that way the variable self.state codes 2 states: the expected token length and how the next token should be interpreted.

I agree that this could be implemented in a completely different way. First grab as many bits until a token is complete, then decode the token. This would also allow to identify aborted tokens. You can find in sigrok-dumps/sdcard/sd_mode/imx6_quad/failing.sr a token starting at 462287 us. It ends at 462320 us although the clock is turned off in between. If you first grab the entire token before decoding it can be possible to detect the token abort.
That trace is captured with a too low sample rate. Therefore the decoder has no chance to find the correct start of token. I don't see why this is not documented in the corresponding README.

[harper23]

@Krakonos I have implemented a redesign of the state machine. Now the token is assembled first before any handler for command or response is called. If the token is complete, the handler for common fields is called. Thereafter the Transmission bit is used to make the decision for the command or response handler.

I have started a branch refactor_sdcard for discussion. I plan to cherry-pick the results into the master branch once we have agreed on it.

I have also added some additional traces to the sigrok-dumps repository and test cases to the sigrok-test repository.
https://github.com/harper23/sigrok-dumps.git
https://github.com/harper23/sigrok-test.git

[Krakonos]

Hi! Sorry it took me a bit to get back to you. I've read through the refactor_sdcard branch and it seems reasonable. I'd suggest two things:

Move the block following if self.token[1].bit: into a function to make it consistent with the else branch that reads with simple self.response.handler(). It would make it more clear we're using just that bit to decide if it's a command or response.

self.expectedBits seems to only be set to default only in reset(), but is later overwritten based on some commands. This is likely fine, but I find this fragile since if error happens (because the target was reset during sampling or something), this will possibly be stuck and will not parse new communication correctly. I'm not quite sure how to handle this, perhaps by calling reset() on error?

Edit: Originally posted this under a wrong account...

[harper23]

I have created a factor handle_command(). This looks similar to calling self.response.handler() although the latter is a call through a reference. At least the purpose is similar.

The original state machine counts all bits at the rising CLK edge. If error happens, this will be stuck in the same way. The data transmission in the SD-card protocol is synchronous. But the frame has start and stop bit, i.e. is asynchronous. If there are gaps between tokens while the clock is still active, the decoder will find a stop bit and the next start bit gives you a new synchronization with the token. Hence, calling reset() on error is not necessary

You can simulate this easily by using a file created with the "Save range as..." menu item. I attach incomplete.zip with an example, one file has an incomplete command. It would be possible to detect this with the new state machine, because it first takes the token and could check the CRC7. You could mark the token is invalid. This is not always simple, since the CRC field is calculated differently depending on the response type. See section "4.9 Responses" of the "Physical Layer Simplified Specification".