nopnop2002 / esp-idf-sx126x

SX1262/SX1268/LLCC68 Low Power Long Range Transceiver driver for esp-idf
MIT License
83 stars 18 forks source link

better error handling #16

Closed Glowman554 closed 1 year ago

Glowman554 commented 1 year ago

Just define a LoRaError(int error) function. If not defined LoRaErrorDefault is used

nopnop2002 commented 1 year ago

Is this return required?

        if(xTaskGetTickCount() - start >= (timeout/portTICK_PERIOD_MS)) {
            ESP_LOGE(TAG, "WaitForIdle Timeout timeout=%lu", timeout);
            LoRaError(ERR_IDLE_TIMEOUT);
            return;
        }
Glowman554 commented 1 year ago

it is there that if the error function returns that the function does not execute code even though a error happened

nopnop2002 commented 1 year ago

I don't understand your content.

LoRaError() is an endless loop and does not return.

void LoRaErrorDefault(int error)
{
    if (debugPrint) {
        ESP_LOGE(TAG, "LoRaErrorDefault=%d", error);
    }
    while (true) {
        vTaskDelay(1);
    }
}

__attribute__ ((weak, alias ("LoRaErrorDefault"))) void LoRaError(int error);

There is no return here.

    if ((GetStatus() & 0x70) != 0x50) {
        ESP_LOGE(TAG, "SetRx Illegal Status");
        LoRaError(ERR_INVALID_SETRX_STATE);
    }
Glowman554 commented 1 year ago

If the user defined function returns. And the other functions do not need to return since it is at the end of the function anyways

nopnop2002 commented 1 year ago

I still don't understand your point of view.

It is no longer safe for the user-defined function to return at this point.

    if ((GetStatus() & 0x70) != 0x60) {
        ESP_LOGE(TAG, "SetTx Illegal Status");
        LoRaError(ERR_INVALID_SETTX_STATE);
    }
Glowman554 commented 1 year ago

I understand that. the return is only there that in case the user decides to ignore the erro (which they shouldnt do) it cant end up in an endless loop

nopnop2002 commented 1 year ago

As for WaitForIdle(), if it doesn't become Idle for a certain period of time, I don't think you can ignore it and continue processing.

I think that what you can do when it does not become Idle for a certain period of time is one of the following.

  1. Permanent loop
  2. Restart
nopnop2002 commented 1 year ago

Now let's go back to the first topic.

The purpose of this assignment was to automatically restart when an error was detected.

What are its advantages?

I don't understand the advantage.

What are your assumptions about the error situation and what happens when it reboot?

Have you ever experienced an actual error and a reboot fixed it?

I've never encountered an error, so I don't understand the situation.

Glowman554 commented 1 year ago

rebooting isnt the only option to handle the error. i would like to exit the task and send a error message in my project. one possible example is the spi transaction error i had

Glowman554 commented 1 year ago

the advantage is that the user decides what happens when a error happend

nopnop2002 commented 1 year ago

i would like to exit the task and send a error message in my project.

this is a good idea.

the advantage is that the user decides what happens when a error happend

understood

Adding returns here should be carefully considered.

void WaitForIdle(unsigned long timeout)
{
    //unsigned long start = millis();
    TickType_t start = xTaskGetTickCount();
    delayMicroseconds(1);
    while(gpio_get_level(SX126x_BUSY)) {
        delayMicroseconds(1);
        //if(millis() - start >= timeout) {
        if(xTaskGetTickCount() - start >= (timeout/portTICK_PERIOD_MS)) {
            ESP_LOGE(TAG, "WaitForIdle Timeout timeout=%lu", timeout);
            LoRaError(ERR_IDLE_TIMEOUT);
            //return;
        }
    }
}
nopnop2002 commented 1 year ago

This allows the user to specify whether to continue, quit, or restart when an error occurs.

#if 0
void WaitForIdle(unsigned long timeout)
{
    //unsigned long start = millis();
    TickType_t start = xTaskGetTickCount();
    delayMicroseconds(1);
    while(gpio_get_level(SX126x_BUSY)) {
        delayMicroseconds(1);
        //if(millis() - start >= timeout) {
        if(xTaskGetTickCount() - start >= (timeout/portTICK_PERIOD_MS)) {
            ESP_LOGE(TAG, "WaitForIdle Timeout timeout=%lu", timeout);
            while(1) { vTaskDelay(1); }
        }
    }
}
#endif

void WaitForIdle(unsigned long timeout)
{
    //unsigned long start = millis();
    TickType_t start = xTaskGetTickCount();
    delayMicroseconds(1);
    while(xTaskGetTickCount() - start < (timeout/portTICK_PERIOD_MS)) {
        if (gpio_get_level(SX126x_BUSY) == 0) break;
        delayMicroseconds(1);
    }
    if (gpio_get_level(SX126x_BUSY)) {
        ESP_LOGE(TAG, "WaitForIdle Timeout timeout=%lu", timeout);
        LoRaError(ERR_IDLE_TIMEOUT);
    }
}
Glowman554 commented 1 year ago

tbh i don't see how that changes anything but i changed it anyways

Glowman554 commented 1 year ago

Anything new?

nopnop2002 commented 1 year ago

I support issues for other libraries.

please wait a little bit.

Glowman554 commented 1 year ago

Anything new?

nopnop2002 commented 1 year ago

Now testing.

Glowman554 commented 1 year ago

Anything new?