`stse_platform_Crc16_Accumulate()` is not safe

[parmi93]

From what I understand, the STSELib middleware allows managing multiple STSAFE devices at the same time through the bus_id parameter. For example, stse_platform_i2c_send_start() takes the integer bus_id as its first parameter, which can be used by the porting layer to determine which STSAFE device to communicate with (in case multiple STSAFE chips are present).

The issue is that the function stse_platform_Crc16_Accumulate() does not take the bus_id parameter, making this function unsafe for this use case, since it requires saving the CRC calculation state in a static variable, which would end up being corrupted.

[nils-cercariolo-st]

Hello, indeed it seems dangerous. Next release will make CRC accumulate re-entrant and safe for multi-device use. But since it's going to be a breaking change, it might not be available so soon. One example to solve it temporarily on your side could be this, only for software implementation :

crc.h:

typedef struct {
	uint16_t value;
} crc16_context_t;

void crc16_Init(void);
void crc16_InitContext(crc16_context_t *ctx);
uint16_t crc16_Calculate(const uint8_t *address, uint16_t length, crc16_context_t *ctx);
uint16_t crc16_Accumulate(const uint8_t *address, uint16_t length, crc16_context_t *ctx);

crc.c:

void crc16_InitContext(crc16_context_t *ctx) {
    if (ctx != 0) {
        ctx->value = CRC_INITVALUE;
    }
}

static uint16_t crc16_update(uint16_t crc, const uint8_t *address, uint16_t length) {
    uint16_t i = 0;

    for (i = 0; i < length; i++) {
        crc = (uint16_t)((crc >> 8) ^ crc16_tab[(crc ^ address[i]) & 0x00ff]);
    }

    return crc;
}

uint16_t crc16_Calculate(const uint8_t *address, uint16_t length, crc16_context_t *ctx) {
    if (ctx == 0) {
        return 0;
    }

    ctx->value = CRC_INITVALUE;
    ctx->value = crc16_update(ctx->value, address, length);

    return (uint16_t)~(ctx->value);
}

uint16_t crc16_Accumulate(const uint8_t *address, uint16_t length, crc16_context_t *ctx) {
    if (ctx == 0) {
        return 0;
    }

    ctx->value = crc16_update(ctx->value, address, length);

    return (uint16_t)~(ctx->value);
}

stse_frame.c:

stse_ReturnCode_t stse_frame_crc16_compute(stse_frame_t *pFrame, PLAT_UI16 *pCrc) {
    stse_frame_element_t *pCurrent_element;
    crc16_context_t crc_ctx;

    if (pFrame == NULL || pCrc == NULL) {
        return STSE_CORE_INCONSISTENT_FRAME;
    }

    pCurrent_element = pFrame->first_element;
    if (pCurrent_element == NULL) {
        return STSE_CORE_INCONSISTENT_FRAME;
    }

    crc16_InitContext(&crc_ctx);

    if (pCurrent_element->length != 0) {
        if (pCurrent_element->pData == NULL) {
            return STSE_CORE_INCONSISTENT_FRAME;
        }
        *pCrc = stse_platform_Crc16_Calculate(pCurrent_element->pData, pCurrent_element->length, &crc_ctx);
    }

    pCurrent_element = pCurrent_element->next;
    while (pCurrent_element != NULL) {
        if (pCurrent_element->length != 0) {
            if (pCurrent_element->pData == NULL) {
                return STSE_CORE_INCONSISTENT_FRAME;
            }
            *pCrc = stse_platform_Crc16_Accumulate(pCurrent_element->pData, pCurrent_element->length, &crc_ctx);
        }
        pCurrent_element = pCurrent_element->next;
    }

    return STSE_OK;
}

There will also be few signatures to change accordingly to the new calculate/accumulate functions, include the crc.h in stse_platform.h (for crc structure). But I think compiler should help you figure out where are the small modifications to make. I unfortunately didn't test if this actually solves our problem but on paper it should I think ! Let me know if you have any more troubles !

[parmi93]

We do not intend to use multiple STSAFE devices simultaneously, but when I reviewed the CRC porting functions these doubts came to mind, and I was wondering whether I had misunderstood the meaning of the bus_id parameter or if there was an issue with the CRC API.
Your reply clarified that I had interpreted the meaning of the bus_id parameter correctly.

[nils-cercariolo-st]

Just to clarify, bus_id is more about peripheral, in case we have multiple I2C buses.
Next release will include some changes only at platform layer. It will include a context at platform layer (one above the current driver context) here but the context will not be at the STSELib layer (in stse_frame.c). So I'm still not expecting it to be fully bullet proof for multi device use or in some specific RTOS configuration.