renesas / fsp

Flexible Software Package (FSP) for Renesas RA MCU Family
https://renesas.github.io/fsp/
Other
192 stars 82 forks source link

r_spi allows a bitrate exceeding max supported speed #296

Closed CraigComberbach closed 11 months ago

CraigComberbach commented 11 months ago

FSP: v4.1.0 Processor: R7FA6M3AH3CFC#BA1 Stack: r_spi

I setup SPI1_A to have a bitrate of 60000000 (60Mbps) with PCLKA = 120MHz, which it accepted and generated the code to do so. The processor did indeed generate a clock with a frequency of 60MHz and seemed to operate as expected during my limited testing (scope bandwidth was too low to do anything but round the edges and prevented any kind of timing analysis to be performed). This is in contrast to the datasheet which states that 30.0Mbps is the maximum supported. Table 38.3 on page 1411 of document R01UH0886EJ0120 Rev.1.20 lists this as an unsupported bitrate.

My expectation as a user of FSP is that it would throw a warning or cap the bitrate to the stated 30Mbps in situations like this.

renesas-brandon-hussey commented 11 months ago

Hi @CraigComberbach , I created a quick project in FSP v4.1.0 and the r_spi driver does report an error if a bitrate > 30MHz is input. A screenshot is below.

image

You are correct that even when we report an error like this, the code will still be generated. This error is in the configurator; it is not a build error. This is a deliberate decision by FSP. The reasoning is that we do not know every way a user will want to use FSP. For this reason we do not want to get in the way of our users. In this case, we inform the user that the bitrate is not valid, but do not stop them from proceeding. Once we have reported the issue, it is is in the user's hands to handle it. This approach is based on user feedback over the years.

The above is related to reporting the issue to the user at build-time. We think this is the best option because users are warned as early as possible. We could implement run-time checks for the bitrate, but once again based on user feedback, we typically do not do so. All run-time code takes code size and execution time. Our users have asked why we need run-time checking when the bitrate was already statically determined at build-time.

CraigComberbach commented 11 months ago

I see it now, the symbol subtly changes and the hover text is indeed there. My only remaining concern is how subtle it was that it is easily missed, but it is there which I am thankful for. Your explanation makes sense and I agree with your reasoning and appreciate the intent to warn a user when doing something not recommended, but to let them do it anyway if they insist.

Thanks for your time!