espressif / esp-modbus

ESP-Modbus - the officially suppported library for Modbus protocol (serial RS485 + TCP over WiFi or Ethernet).
Apache License 2.0
107 stars 49 forks source link

mbc_master_send_request - Exception Codes (IDFGH-12757) #60

Closed dmglogowskiOS closed 4 months ago

dmglogowskiOS commented 5 months ago

When mbc_master_send_request returns an ESP_FAIL to the caller, the documentation states that an exception or other failure occurred. However, it is unclear how to determine what might be the case, since:

  1. ESP_FAIL indicates that either an exception occurred OR that there was another failure
  2. It is not clear if the modbus exception codes are accessible, when an exception occurred.

Clarifying information, and adding to the documentation, about this would be helpful, since the documentation is currently quite ambiguous in this regard.

alisitsyn commented 5 months ago

@dmglogowskiOS,

Thank you for this issue. It is a very good question. Unfortunately, the current API do propagate the error code to indicate an exception but do not provide the way to determine the exact code of exception and the way of handling for this. I don't have nice solution for this but can provide the simple workarounds for this issue. One solution will not involve the changes in the library code. I will provide update ASAP.

alisitsyn commented 4 months ago

@dmglogowskiOS,

I propose the workaround to add the user error handler function to catch the exception code. It also allows to check the data buffers on user side (this can be useful for some gateways).

Please take a look and let me know if this would work for you. Thanks. T

dmglogowskiOS commented 4 months ago

This may work quite well for most users imo, but I am unsure if it would be the best solution for my use case since I am working on a more object oriented (and C++ based) library based on esp-modbus and I am unsure how to implement this error handler in a manner that is compatible with my approach, although I do believe it should be possible.

alisitsyn commented 4 months ago

I am unsure if it would be the best solution for my use case since I am working on a more object oriented (and C++ based) library based on esp-modbus and I am unsure how to implement this error handler in a manner that is compatible with my approach, although I do believe it should be possible.

I understand your requirement. It is still possible to implement this with the callback but can agree this is like workaround but not the nice approach to do. The best approach would be to propagate the possible exception codes to the subset error codes returned from mbc_serial_master_send_request() but the original idea was to hide Modbus specifics (mb exceptions are) from higher layer and I would not want to change it now. Other way could be to implement some additional API to get the exception code related to latest completed Modbus IO request. This would change the API interface but in compatible way. Also, the approach selected for this needs to be implemented the same way in v2.0 stack. I am still thinking about better way for this. You can also propose your view to this.

dmglogowskiOS commented 4 months ago

I believe a possible way to handle this might be by adding either a function mbc_serial_master_last_return() which should return the last state (Either 0 or the exception code of the last exception) or adding an overload to mbc_serial_master_send_request(), either by making it an option configurable in the menuconfig or by adding a separate function (mbc_serial_master_send_request_ret() maybe?) where in addition to the request and the data, you also pass it a pointer to a return/metadata buffer.

What could also work is by maybe refactoring the function to return this struct:

typedef struct {
    const esp_err_t err;
    const uint8_t function_code;
    const uint8_t exception_code;
} slave_return_t

The caller can then read the error code from that structure and handle any exception accordingly, if the error code signifies an exception occured.

What may definitively be needed is an error code excplicitly alerting the program that an exception has occured, rather than ESP_FAIL...

Obviously this second approach would definitely break the current API.

alisitsyn commented 4 months ago

Obviously this second approach would definitely break the current API.

This way will not be implemented.

I believe a possible way to handle this might be by adding either a function

Your approach is a bit different with what I mentioned the function mbc_master_get_transaction_info

dmglogowskiOS commented 4 months ago

That function would be almost exactly what I need and I could easily integrate this into my library

alisitsyn commented 4 months ago

Ok, good. This will be updated accordingly and let us keep it as a basic approach. I would keep the callback approach as well, because it can be useful in some other user use cases.

alisitsyn commented 4 months ago

This old CPP wrapper will probably help you.

dmglogowskiOS commented 4 months ago

Thanks but see my answer in the associated Repo.

alisitsyn commented 4 months ago

The issue has been fixed with commit 1eafc29df3d14ec379ea76f07337ae9816443e75

alisitsyn commented 4 months ago

The component release is a bit postponed for now.