Add method to monitor for state changes

[rovo89]

... and a couple of (breaking) changes to make using the new method (but also other scenarios) easier to use.
Fixes #5.

[sschum]

Thanks for participating again ;)

[sschum]

I guess you save the state internally to prevent to inform the "StateChangeHandler" multiple times... This is actually the only reason why you "really" need an nuki-Client in MonitorStateChanges(...). In my opinion the multi-inform-issue should be an issue for the "StateChangeHandler" and not an issue for the nuki-library. If you "outsource" the issue to the "StateChangeHandler" you only need a ble-device-address and must not establish a "whole" nuki-client.

[rovo89]

you only need a ble-device-address and must not establish a "whole" nuki-client.

For the pure monitoring, yes. However, literally the only thing the advertisement bit tells you is that any state has changed. You'll need to call readState() to learn which of the states it was and what the new state is - and for that you'll need a full client. I don't think there'll be any script which can work without a client, and if you create it anyway, then why not pass it?

If you just gave a device address to the monitoring method, then the callback could also tell you only the address. You would have to create a new client every time, pass the authentication details again etc. Or you would need your own lookup map to find your pre-created client for that address.

The way I implemented it, you can create the client once in the beginning of your script, set it up with all the details(*) and then start monitoring it. The callback will be triggered with that ready-to-use-client as parameter, so you can immediately establish a connection, call readState() and whatever else you want.

I'm all for keeping things flexible, but again, I don't see the need to "save" the creation of a client and would rather make it easy to use.

---

(*) I have prepared (but not yet pushed) a helper for easier (un-)marshalling from/to JSON and YAML, so you can keep the address and credentials in a config and easily create the client(s) from that.

[rovo89]

In my opinion the multi-inform-issue should be an issue for the "StateChangeHandler" and not an issue for the nuki-library.

At least my Nuki Smart Lock 3.0 sends BLE advertisements about once per second. Each of them includes the flag, so as soon as someone e.g. unlocks via app, the callback would be triggered every second. I can't think of any use-case where a script would need repeating notifications, because again, there isn't any state encoded in the bit itself. Therefore I think the library should take care of it.

[rovo89]

I thought about the Addr vs. Client topic a bit more. Actually the Client struct could be split into two groups of fields:

1. Static information/configuration
   • addr (added by me)
   • privateKey
   • publicKey
   • nukiPublicKey
   • authId
   • pin (potential future extension)
2. State/connection resources:
   • responseTimeout (although that's somewhere middle ground and could go in either section)
   • client
   • gdioCom
   • udioCom

That's obvious when we look at Close() - only the second group of fields is reset there. The first group of fields exists even when disconnected, it's just a few bytes in main memory, which is far cheaper to create than something involving the Bluetooth stack.

For a typical scenario with monitoring, the local Bluetooth device will be in scanning state most of the time, and will only establish a connection for a short time when either the lock signals that its state has changed or when some action (e.g. unlock) has been requested by the user. For this scenario, the struct with the static information would be created once when the script starts, and the connection struct would be created and destroyed only when needed.

Looking at go-ble, they have a similar thing: You have the Addr as configuration and receive a Client from Dial(). Following the same approach, this library could have the above field groups as Config and Client structs. Client would have a reference to the underlying Config. Most existing methods would stay in Client, biggest exception being EstablishConnection() which would have to be called on Config and would create a new Client.

Not sure if you'd be ready to break backwards-compatibility that much. It would separate concerns and might make it easier to justify that a Config (but not a Client) should be used to monitor devices. Maybe I'll just go ahead and prepare a PR to see what the result looks like.

[rovo89]

Maybe I'll just go ahead and prepare a PR to see what the result looks like.

Check out https://github.com/rovo89/go-nuki/commits/split_client. Maybe not completely done yet, but should give a rough idea into which direction it could go. Btw, Config might be a bad name because it could be confused with ReadConfig(). But I couldn't think of anything better, only Device, but that's also used be go-ble for the local Bluetooth devices...

[sschum]

Yeah maybe you are right: there is no scenario which the user wants to only inform about changes without knowing what changed. Therefore you can leave it how it currently is.

But i will leave some code comments for the current approach.

[sschum]

I would not track the change-state in a nuki-client. But maybe in a own internal map which is only used by Monitor. So i would prefer to implement a struct named "Monitor" with an Method which starts the monitoring. Something like that:

type monitor struct {
	states map[string]bool //ble-add -> stateChanged
}

func NewMonitor() *monitor {
	return &monitor{
		states: map[string]bool{},
	}
}

func (m *monitor) MonitorStateChanges(ctx context.Context, clb StateChangeHandler, addr ...ble.Addr) error {
...
	m.states[addr.String()] = stateChanged
...
}

[rovo89]

Is there any specific reason why this needs to be in a struct? Even if you want to decouple this from the client, it could as well be a local variable in that method.

There's another motivation to keep it in the client though: Automatically re-enabling the notifications when readStates() has been called. The only other way I can see this happing is to assume that the callback will always call readStates(), and therefore to reset the flag when the callback has finished. But if the callback decides to postpone the states retrieval, the lock will keep sending the flag and the library will think it's a new event...

[sschum]

No your are right: there is no need for a own struct. Which brings me to another point, though:

The current implementation will "observe" clients which are configured at the start of monitoring. What if your application will create another client after the monitoring is starting. Therefore you have to stop, and start the monitoring again with the new client. Maybe it is a better approach to make an own struct which can hold the "observed" clients, where you can add/remove clients at runtime:

func NewMonitor() *monitor {
	return &monitor{
		watchedClients:      map[string]*Client{},
		watchedClientsMutex: sync.RWMutex{},
	}
}

func (m *monitor) ObserveClient(clients ...*Client) {
	m.watchedClientsMutex.Lock()
	defer m.watchedClientsMutex.Unlock()

	for _, client := range clients {
		m.watchedClients[client.addr.String()] = client
	}
}

func (m *monitor) StopObserveClient(clients ...*Client) {
	m.watchedClientsMutex.Lock()
	defer m.watchedClientsMutex.Unlock()

	for _, client := range clients {
		delete(m.watchedClients, client.addr.String())
	}
}

// MonitorStateChanges listens to advertisements from the given devices and triggers the callback
// once their state changes. The client - which must be paired with ClientIdTypeBridge - can then
// call ReadStates to fetch the new state and reset the flag.
func (m *monitor) MonitorStateChanges(ctx context.Context, clb StateChangeHandler) error {
	h := func(a ble.Advertisement) {
		m.watchedClientsMutex.RLock()
		c := m.watchedClients[a.Addr().String()]
		m.watchedClientsMutex.RUnlock()

		if c != nil {
			_, stateChanged := ParseAdvertisement(a)
			if stateChanged && !c.stateChanged {
				go clb(ctx, c)
			}
			c.stateChanged = stateChanged
		}
	}
	return ble.Scan(ctx, true, h, nil)
}

[sschum]

The name "MonitorStateChanges" sounds to me that this method will permanently monitor devices. But you only call once the ble.Scan method. Will the ble.Scan() scan infinity until the context is close? Or does it only one "scan-cycle" 🤔

[rovo89]

The ble.Scan() method will keep scanning until you cancel the context. The MonitorStateChange function intentionally has the same behavior - it's up to the user to pass a context with a timeout or explicitly cancel it on a certain event.

[sschum]

Ok than it should be reflected in comment and/or in the example. Maybe you can change the example with an context with timeout:

....
ctx, cancelFn := context.WithTimeout(context.Background(), 10*time.Second)
defer cancelFn()
	
MonitorStateChanges(ctx, handleStateChange, nukiClient)
....

[sschum]

here you should add a comment why you are using this byte-range -> I would like to see a structure of the package. Similar to crypto.go if possible 😇

[rovo89]

Will see what I can do. I had hoped that go-ble provides a parser for iBeacons, but even though it has methods to send them, I didn't find any parsers.

[sschum]

for the example_test.go i would prefer to use an inline function as callback:

Suggested change

	MonitorStateChanges(context.Background(), handleStateChange, nukiClient)
	MonitorStateChanges(context.Background(), func(ctx context.Context, c *Client) {
	fmt.Println("State change detected, loading new state...")
	defer c.Close()
	err := c.EstablishConnection(ctx)
	if err != nil {
	panic(err)
	}
	states, err := c.ReadStates(ctx)
	if err != nil {
	panic(err)
	}
	fmt.Printf("Device-State: %s\n", states.String())
	}, nukiClient)

[rovo89]

OK, will change this. I thought that in "real life", you'd probably want to use a separate function.

[sschum]

Maybe you can add that "real life" example into the README 👍

[sschum]

Has it to be a public method? If yes: please add a documentation. Otherwise make it private (the name of the function must be start with lowercase)

[sschum]

Sorry for late response! 😓
I think we should not overestimate the backwards-compatibility at this state. Because your PullRequest contains some compatibility issues, we will make an V2 branch after merging this ;)

[rovo89]

Thanks for your reply! I'll read through them later or in the next few days.

Did you also have a look at https://github.com/rovo89/go-nuki/commits/split_client?
The idea behind it is described here: #8 (comment) and #8 (comment)

[sschum]

Now I have looked into this split_client approach: For the first steps it looks good, but i think we have to order the code a bit more ;) (i known that was a proof-of-concept). Maybe the config building can be more "sophisticated" by using the builder-pattern:

NewConfig(<addr>).WithPin(<pin>).WithTimeout(<timeout>).EstablishConnection()

I have implements such builder pattern in one of my private project: https://github.com/rainu/go-command-chain ... and it is fun to use those builder-functions. (but i know that this is not easy every time ;) )

Maybe you can make another pull request and talk to this later :)