New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PJONMaster - Arduino reboots when packet delivery fails #106

Closed
elusive-code opened this Issue Dec 8, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@elusive-code

elusive-code commented Dec 8, 2016

When MAX_ATTEMPTS reached arduino goes into reboot.

It looks like it happens because _master_error is never set, so when error_handler invokes it - null dereferenced and arduino goes down.

(not sure if the same is valid for PJONSlave, but from the look of the code - it should be)

Apparently PJONMaster constructor invokes set_error of parent class PJON, which set it's own _error field.
While set_error method of PJONMaster sets _master_error, and is never invoked.

I think separate error handler field in PJONMaster and PJONSlave is a unnecessary, it's probably better to make _error field protected, and invoke it.

Another thing I noticed that may cause trouble: PJONMaster and PJONSlave do not invoke set_default (as well as parent constructor).

@gioblu

This comment has been minimized.

Owner

gioblu commented Dec 8, 2016

Ciao @elusive-code I have run a fast test without oscilloscope not having enough time to run an extended analysis. What I see is that:

  • on the slave's side the if 3000 millis forces the slave not to receive after 3 seconds and should be removed to do this test
  • that on both sides receive should be called passing a timeframe to higher the time dedicated to reception, because as it is proposed in the example, receive tries only once to get a packet.
  • defining the error function and passing it to set_error (https://github.com/gioblu/PJON/blob/master/documentation/error-handling.md) I can see prints so it seems to be called correctly
  • Looking at the master prints it seems it doesnt reset, because it keeps to print the older ids.
  • After error call the lost id seems effectively deleted because disappear from the list printed by master

What it is not nominal, is that after error the slave for some reason is forced to drop its id and get a new one, creating a bad chain reaction :). Maybe was this behaviour forcing to think about a reset?

So, I will get the code, some time and the oscilloscope and will debug this.

I agree with you that the error is not the best implementation possible (consider that those two classes are experimental and for this reason still not documented), feel free to propose a better solution!

@gioblu gioblu added the bug label Dec 8, 2016

@gioblu gioblu changed the title from Arduino reboots when packet delivery fails to PJONMaster - Arduino reboots when packet delivery fails Dec 8, 2016

@elusive-code

This comment has been minimized.

elusive-code commented Dec 9, 2016

Hi, @gioblu, sorry, I wasn't clear in the report.
I didn't invoke set_error manually from my code, from my understanding it is optional.
At least there is dummy_error_handler, and it is set as error handler in PJON, but not in PJONMaster.

  • If you will look at the PJONMaster constructor, you will see it invokes
    PJON<Strategy>::set_error(static_error_handler);
    but it doesn't set its own _master_error field (it is left uninitialized).

  • static_error_handler invokes
    master->error_handler(code, data);

  • and error_handler invokes
    _master_error(code, data);
    which was not initialized, and thus at that point arduino goes into reboot.

It looks that same should be valid for PJONSlave:
constructor
static_error_handler
error_handler

Here is a code I used to configure master:

    PJONMaster<SoftwareBitBang> bus(server_bus_id);
    ...
    bus.strategy.set_pin(3);
    bus.include_sender_info(true);
    bus.set_receiver(receiver_function);
    bus.begin();
@gioblu

This comment has been minimized.

Owner

gioblu commented Dec 9, 2016

@elusive-code thank you for your analysis I will dedicate some time today to fix this, and to hopefully provide with a fix for tomorrow!

@gioblu

This comment has been minimized.

Owner

gioblu commented Dec 10, 2016

CIao @elusive-code thank you for your report.
It now works flawlessly with the referenced changes.

@gioblu

This comment has been minimized.

Owner

gioblu commented Dec 11, 2016

@elusive-code will close here considering fixed.
Thank you for your support! Happy tinkering.

@gioblu gioblu closed this Dec 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment