Add data_input_ignore config to ignore idle values before insertion into queue_rx

schang412 / cocotbext-spi

MIT License

20 stars 10 forks source link

Add data_input_ignore config to ignore idle values before insertion into queue_rx #8

Closed qcabanes closed 1 year ago

qcabanes commented 2 years ago

Their should be a data_input_idle config for SPIMasters so that MISO idle words is skipped from being queued into queue_rx. This mean that we do not need to clear queue_rx of empty values from the idle words of the SPISlave.

schang412 commented 2 years ago

Do you mean to say that indicates data_input_idle that we should ignore the input data? If so, we should call it data_input_ignore.

qcabanes commented 2 years ago

Yes, that would be perfect !

schang412 commented 2 years ago

Looking at the code, I see that the master will not have to call master.clear() with this feature. But, there's nothing that requires to read the data from the master. I didn't design the spi extension with the ability to change config on the fly in mind, so I am not sure what advantage this change would give.

qcabanes commented 2 years ago

Well, my use case is the following. I have two IPs communicating through SPI. The master is sending data to the other and wait for the processing. When the processing is finished, an interuption is raised and the master will talk to the slave to get the data back. What happens here is that the queue_rx of the master is full of idle bytes from the slave and needs to be cleared before reading meaningful bytes. IMHO, it does not make sense to clear this buffer full of meaningless information. So a way to skip those idle bytes would mean no need to clear the queue_rx buffer of the master SPI.

schang412 commented 2 years ago

So, if I am understanding correctly, a use case would be

spi_master.update_config(ignore_rx=True)
await spi_master.write(0x00)  # spi slave is returning junk
spi_master.update_config(ignore_rx=False)
await spi_master.write(0x01)
result = await spi_master.read()

The issue is that currently, updating a configuration on the fly is not a supported/tested functionality, and I don't see much advantage over:

await spi_master.write(0x00)  # spi slave is returning junk
spi_master.queue_rx.clear()
await spi_master.write(0x01)
result = await spi_master.read()

If this is a feature that you would like to see, I think the first step is making sure changing the configuration on the fly does not break anything.

qcabanes commented 2 years ago

This use-case seems great but I do not think the on-the-fly reconfiguration is necessary at first:

spi_config = SpiConfig(
    ignore_rx_value = 10 # Ignore all RX values that are equal to 10
)

And simply add in spi.py#L224:

if self._config.ignore_rx_value != None and rx_word != self._config.ignore_rx_value:
    self.queue_rx.append(rx_word)

I personally see two pros:

Less RAM usage (because we do not store RX values)
Cleaner code (but this is a personal opinion that may not be interesting to consider, I just feel its a bit weird to clear the RX buffer before writing to slave)

Cons:

It can be misused by developer if not understood correctly

schang412 commented 1 year ago

Sounds good. Could you make a pull request with this addition, including tests and documentation.