Connection Event shouldn't be called when cancelling connection from within the app

[julianvogels]

Issue

I am trying to distinguish two cases:

1. The peripheral disconnects because the connection broke or it was otherwise disconnected with the device controls
2. The peripheral disconnects due to a user action or automatic action within the app

Previously, using another library (RKBluetooth), the peripheral connection had a callback block for completion of connection and for disconnection. The disconnection block would only be called when the device disconnected by itself.

I am missing this feature on RZBluetooth, maybe I just didn't discover it yet. I don't understand the relationship between the callback block of the connection or cancel connection methods of the RZPeripheral, and the connectionEvent delegate callback.
Both are executed, while the method callback blocks are executed before the delegate method.

I wonder what the preferred way of handling this distinction is? I don't want to store a flag when the method callback block is called so that I can prevent the connectionEvent to execute, which is the only workaround I can think of right now.

[KingOfBrian]

Yea, that makes sense, but I'm not sure exactly how to change this behavior without shaking up the contract. I think adding RZBPeripheralStateEventUserCancelled would make sense and basically doing the flag management you mention internally. If you wanted to make a PR It would be great, otherwise I think tracking the state as you mention might be your best case in the short term.

Just to think out loud here -- The connection management really needs some cleanup, it "evolved" a bit, and I don't really like the current shape. My current thinking is:

• replace the connectionDelegate with a block onConnectionChange that takes a RZBPeripheralStateEvent and optional error.
• Remove the maintainConnection property, and replace with a -[RZBPeripheral attemptReconnectionForEvent:(RZBPeripheralStateEvent)]. User would explicitly call this in the onConnectionChange block to maintain a connection attempt. The UserCancelled state helps here too since if you cancel the connection, this should stop the retry.
• Remove - (void)connectionEvent:(RZBPeripheralStateEvent)event error:(NSError * __nullable)error; all together -- just use the block.

I haven't had the time to fully think through things though. Chip in if you have any thoughts (@cpatterson-lilly) I'm not active on a bluetooth project, so I'd love some help with an update!

[cpatterson-lilly]

Hey @KingOfBrian, I'm back from dub dub and got time to think about this for a minute.

I agree with your design proposal; one of the key benefits of RZBluetooth is that it provides a block-based interface to everything, so getting rid of connectionDelegate makes a lot of sense to me.

However, I think maintainConnection still makes sense for some devices -- if enabled, onConnectionChange would just see the disconnect/reconnect events as they occur. If disabled, then the additional attemptReconnection... method could be used in the block as you said.

If attemptReconnection... is called when maintainConnection is already set, I think it becomes a no-op. (Or would the command dispatch queue automatically de-dup the connection requests?)

Not sure if I'll have time to do a PR this time -- dub dub dropped a lot of stuff I need to investigate!

[tahirmt]

Hey guys @KingOfBrian @cpatterson-lilly while I do like the idea of having a RZBPeripheralStateEventUserCancelled I do not think you should get rid of the delegate entirely. Because for keeping multiple peripherals connected monitoring each peripheral's state the delegate is extremely useful. I am currently doing that in a project. Without the delegate, each peripheral object would have to maintain a separate block which could make things a little messy.

I have some time and I think I can implement it.. I'll let you guys know when I'm done.

[tahirmt]

Okay so I've made the following changes and since I'm not a collaborator I cannot create a PR

In RZBPeripheral.h

/**
 This block will be triggered whenever the peripheral connection state changes. 
 Use this as an alternative to RZBPeripheralConnectionDelegate
 */
@property (nonatomic, copy) RZBConnectionBlock connectionEventHandler;

/**
 Attempt reconnection for the provided event. 
 Use this to maintain a connection with your peripheral.

 @param event the event to attempt reconnection for
 */
- (void)attemptReconnectionForEvent:(RZBPeripheralStateEvent) event;

In RZBPeripheral.m

- (void)connectionEvent:(RZBPeripheralStateEvent)event error:(NSError * __nullable)error;
{
    [self.connectionDelegate peripheral:self connectionEvent:event error:error];

    if (event != RZBPeripheralStateEventConnectSuccess && self.maintainConnection) {
        [self connectWithCompletion:nil];
    }
    
    if (self.connectionEventHandler) {
        self.connectionEventHandler(event, error);
    }
}

-(void)attemptReconnectionForEvent:(RZBPeripheralStateEvent)event {
    if (event != RZBPeripheralStateEventConnectSuccess && event != RZBPeripheralStateEventUserCancelled) {
        [self connectWithCompletion:nil];
    }
}

Added the state event

typedef NS_ENUM(NSUInteger, RZBPeripheralStateEvent) {
    RZBPeripheralStateEventConnectSuccess,
    RZBPeripheralStateEventConnectFailure,
    RZBPeripheralStateEventDisconnected,
    RZBPeripheralStateEventUserCancelled,
};

Finally, changed the -centralManager: didDisconnectPeripheral: error: delegate method as follows

- (void)centralManager:(CBCentralManager *)central didDisconnectPeripheral:(CBPeripheral *)corePeripheral error:(NSError *)error
{
    RZBLogDelegate(@"%@ - %@ %@ %@", NSStringFromSelector(_cmd), central, RZBLogIdentifier(corePeripheral), error);
    RZBPeripheral *peripheral = [self peripheralForCorePeripheral:corePeripheral];

    BOOL didUserCancel = [self completeFirstCommandOfClass:[RZBCancelConnectionCommand class]
                     matchingUUIDPath:RZBUUIDP(corePeripheral.identifier)
                           withObject:corePeripheral
                                error:error];

    // This delegate method can terminate any outstanding command, and is often the terminal event
    // for a connection. Fail all commands to this peripheral
    NSArray *commands = [self.dispatch commandsOfClass:nil
                                      matchingUUIDPath:RZBUUIDP(corePeripheral.identifier)
                                            isExecuted:YES];
    for (RZBCommand *command in commands) {
        [self.dispatch completeCommand:command withObject:nil error:error];
    }
    // Clear out any onUpdate blocks
    [peripheral.notifyBlockByUUIDs removeAllObjects];
    RZBPeripheralStateEvent state = didUserCancel ? RZBPeripheralStateEventUserCancelled : RZBPeripheralStateEventDisconnected;
    
    [peripheral connectionEvent:state error:error];
}

This adds the required features but does not remove anything (yet)...

[cpatterson-lilly]

@matsoft90 There is no explicit "collaborator" role on Github. I am a "collaborator" only in the sense that @KingOfBrian has accepted some of my PRs.

To create a PR, you must "fork" RZBluetooth into your own account, make the above changes to your copy of the repo, then submit the PR back to the original repo.

See Fork A Repo to get started...

[tahirmt]

@cpatterson-lilly I'm relatively new to GitHub. I thought all I had to do was create a new branch and create a pull request. Thank you.

I have created the PR with the above changes. @KingOfBrian please review

[KingOfBrian]

Fixed in #95!