search

getsenic / nuimo-windows

Library for Windows 10 platforms to connect and communicate with Nuimo controllers made by Senic
MIT License
23 stars 8 forks source link

Get Battery and Firmware version on request #10

Closed Inrego closed 8 years ago

Inrego commented 8 years ago

Currently, the only way to get firmware version is by hooking up to the get firmware event. One could argue that it makes sense, since it doesn't change during a connection session (device needs to reboot and reconnect to update). But for battery it's the same (except there's an event to get notified of changes). But I would like to be able to request battery level (and for the sake of completion, I just added firmware too). I see no reason why these methods should not be available for public consumption (unless requesting current battery % is a very expensive operation that should really be limited to only once per session).

Inrego commented 8 years ago

After re-reading the comment, I noticed I used a lot of parenthesis. Please bear with me :smile:

Inrego commented 8 years ago

On afterthought, maybe it should be implemented similarly to DisplayLedMatrixAsync as to making the call async. I can't decide which I like better:

Any comments?

larsblumberg commented 8 years ago

The SDK already has facilities to report battery change events. It also reads the battery value and the firmware version on connection and then sends a BatteryPercentageChanged and FirmwareVersionRead event.

Why are the existing facilities not sufficient for you?

larsblumberg commented 8 years ago

I'm closing this PR for now – still happy to discuss your thoughts, though!

Inrego commented 8 years ago

Lets say there is a shaky bluetooth connection, and the battery changed event doesn't go through. Then the software has the incorrect battery value.

Another example (which is the primary reason in my case): I am developing a service that acts as a proxy between Nuimo and a python app (python doesn't really have much in the ways of Bluetooth on Windows). The service automatically connects Nuimos and keeps an internal list of which ones are connected. Besides that, all it does is relay events to connected python clients, and allowing python clients to call functions on the Nuimo (for example set led matrix). In my opinion, it is out of the scope of this service to keep track of battery levels of the connected Nuimos, and there currently are no ways for python clients to request current battery level of the Nuimos, since it is only being made available upon connection start, which was when the service started and not when the python client connected.

I know it could be solved by developing my service to monitor current percentage levels, but I see no reason why we shouldn't be able to request current battery level, when the information is available to be requested from the Nuimo.

You could also say that it's a part of the Nuimo bluetooth api that is not implemented in this SDK, and therefore this SDK is not feature complete.

I think deep down, this also boils down to the discussion we had about thread management on GUI's. When making the SDK, you thought of how you wanted to use the SDK, and made the SDK for that. Doing so, you implemented platform specific methods, and limited implementation of all functionality. Instead (in my opinion, I'm not saying my opinion is the only way), an SDK should first and foremost be "dumb" and just implement anything the device offers, with no bells and whistles. When that's done, then helper classes, logic etc can be built as an abstraction layer on top of it.

That's just my 2 cents. I have my own fork, so it's not like I need you to implement this. I just think others might want to be able to request current battery level, since I ran into wanting to do so :smile:

hansmbakker commented 8 years ago

@larsblumberg I really would expect of a company's official SDK that it is feature complete.

Adding features does not necessarily make a library more complex. The .Net framework has thousands of classes, methods, properties. Still - beginning users do not need to be overwhelmed by that if they start with the basics, read tutorials etc.

So actually I think this PR has a good concept - it makes available the functionality that Nuimo offers. That there is also an event for that is not a reason not to include it, IMHO.

An analogy: most GUI applications (a browser or desktop window) have properties for the window size. These properties can be read directly (as @Inrego proposed). Besides these properties, there is usually also an OnResize event. Yet, I honestly never heard any (beginning) developer complain that there were too many ways to find out about the window size...

@Inrego asked: (unless requesting current battery % is a very expensive operation that should really be limited to only once per session)

This question wasn't answered, but I can imagine that sending too much bluetooth traffic can influence the battery life. So this function to request the battery charge could get a remark in its documentation to point people to that.

I hope that you'll give @Inrego a chance to complete his PR by reopening it. I believe having his work in your repo is much more valuable for everybody than having separated forks where people work separately.