nopnop2002 / esp-idf-mirf

nRF24L01 Driver for esp-idf
MIT License
59 stars 10 forks source link

Added the option to deinit #12

Closed nguterresn closed 2 months ago

nguterresn commented 2 months ago

Title. Not a big change but I eventually had the need for it

nopnop2002 commented 2 months ago

Why is Nrf24_deinit missing from mirf.h?

Have you set SPI_Frequency to 10000000 Hz and confirmed it works correctly? And What are the positive effects of setting SPI_Frequency to 10000000 Hz?

nguterresn commented 2 months ago

Why is Nrf24_deinit missing from mirf.h?

I forgot. I'll add later on.

Have you set SPI_Frequency to 10000000 Hz and confirmed it works correctly? And What are the positive effects of setting SPI_Frequency to 10000000 Hz?

No, I haven't. I basically ported it based on the comments.

nopnop2002 commented 2 months ago

Unfortunately, we cannot accept PRs that have not been confirmed to work properly.

We will take your opinions into account.

nguterresn commented 2 months ago

Unfortunately, we cannot accept PRs that have not been confirmed to work properly.

I said I'd do it later. What you mean? I clearly contributed in the best of intentions.

Apart from the missing declaration in the header, everything works — there are no breaking changes.

If you made this repository public to act like this, you might just as well make this private and use it to yourself only.

nopnop2002 commented 2 months ago

Apart from the missing declaration in the header, everything works

Setting SPI_Frequency to 10000000 Hz obviously doesn't work with ESP32

Please check in your own environment.

It works correctly when SPI_Frequency is 8000000 Hz, but what is the benefit of this?

Please let us know why you made the SPI frequency changeable.

We have confirmed that the ESP32/ESP32S2/ESP32S3/ESP32C2/ESP32C3/ESP32C6 operates properly at an SPI frequency of 4000000Hz. If we change the SPI frequency, we will need to re-verify operation on these SoCs. It takes a lot of time.

there are no breaking changes.

The following changes have a significant compatibility impact. Validation of the following changes will take a lot of time.

    config SPI_FREQUENCY
        int "SPI Frenquency"
        range 4000000 10000000
        default 4000000
        help
            SPI Frenquency in Hz.
nguterresn commented 2 months ago

//static const int SPI_Frequency = 10000000; // Unstable even with a short jumper cable

I suppose this comment was left by you? I've based my assumptions in your code debt leftovers, which I shouldn't have done.

The default value is still 4MHz and the limit can be decreased from 10Mhz to 6 for example. Or — to make things a bit more idiomatic — the value is not mutable. However, this means cleaning all the existing comments about different SPI frequencies. I'm OK with any.

Also, stating from the datasheet:

0-10Mbps 4-wire SPI

Therefore, according to the official documentation, it is not that farfetch to provide 10MHz as the SPI frequency. However, I understand it might not work for every case and thus, it loses its purpose.


How do you normally validate the code?

nopnop2002 commented 2 months ago

How do you normally validate the code?

Test every possible case.

    config SPI_FREQUENCY
        int "SPI Frenquency"
        range 4000000 10000000
        default 4000000
        help
            SPI Frenquency in Hz.

With this change, We need to test that it works in all the cases below. Errors may occur unexpectedly.

ESP32 at 4000000Hz ESP32 at 6000000Hz ESP32 at 8000000Hz ESP32 at 10000000Hz --> this doesn't work

ESP32S2 at 4000000Hz ESP32S2 at 6000000Hz ESP32S2 at 8000000Hz ESP32S2 at 10000000Hz

ESP32S3 at 4000000Hz ESP32S3 at 6000000Hz ESP32S3 at 8000000Hz ESP32S3 at 10000000Hz

ESP32C2 at 4000000Hz ESP32C2 at 6000000Hz ESP32C2 at 8000000Hz ESP32C2 at 10000000Hz

ESP32C3 at 4000000Hz ESP32C3 at 6000000Hz ESP32C3 at 8000000Hz ESP32C3 at 10000000Hz

ESP32C6 at 4000000Hz ESP32C6 at 6000000Hz ESP32C6 at 8000000Hz ESP32C6 at 10000000Hz


void Nrf24_deinit(NRF24_t *dev) {
    memset(dev, 0, sizeof(NRF24_t));
    spi_bus_free(HOST_ID);
}

Please give me a sample code how to use this function. Obviously, this change does not affect existing processing.

nguterresn commented 2 months ago

Look at how I deinit different "applications".

nopnop2002 commented 2 months ago

Thank you for providing the sample code.

Added Nrf24_deinit function.

https://github.com/nopnop2002/esp-idf-mirf/tree/master/components/mirf

nguterresn commented 2 months ago

Thank you :)

nopnop2002 commented 2 months ago

Your PR was very helpful. thank you.