Skip to content
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

Determine micro:bit model version and add to Device Information Service #373

Open
wants to merge 2 commits into
base: dal-integration
Choose a base branch
from

Conversation

microbit-sam
Copy link
Contributor

The micro:bit can detect what kind of micro:bit it is: 1.3x, 1.5x using by determining which accelerometer is in use.

This is then appended to "BBC micro:bit" in the Device Information Service.

@microbit-sam
Copy link
Contributor Author

Example program here:

microbit-samples-combined.hex.zip

@ghost
Copy link

ghost commented Aug 20, 2018

What about the future though? Will this essentially be binary versioning i.e. if it's 1.3 or 1.5 depending on the accelerometer? What if there are other editions which retain the current accelerometer?

@microbit-sam
Copy link
Contributor Author

I think if we have to update the board to revise another sensor or similar in the future, we should be able to identify it by the ICs on board / something similar

It should be fairly simple to modify the getModel function in MicroBit.cpp to query and identify a micro:bit's model depending on any future changes

https://github.com/lancaster-university/microbit/pull/12/files#diff-6047755bfccc64c91f0d390dd2f1061eR253

@microbit-sam
Copy link
Contributor Author

@martinwork
Have you got any thoughts on this?

@martinwork
Copy link
Contributor

It would be more convenient if this info was in the advertisement, but the problems that might cause for existing software parsing the advertised name have already been discussed!

Thinking about the above and @bluetooth-mdw 's comment, it would be good to make the format explicit now and specify that other info could be added, so apps can allow for that when parsing. How much space is available?

Integer version numbers are slightly easier to deal with than float or text. 13 and 15? I don't know the significance of these actual version numbers. If we started with 0 and 1 now, it would fit it into a byte for 700+ years at the current rate!

@ghost
Copy link

ghost commented Aug 23, 2018

Agree wth @martinwork

Also... the version information is of no use unless the distinction between micro:bit versions is going to be documented and there's a commitment to do that into the future. Where will this documentation live? "Documentation" may be the wrong word.... it's perhaps no more than a paragraph per version but whatever form it takes, it's needed.

@martinwork
Copy link
Contributor

Definitely specify the format in code comments.
How about adding an indication of application vs pairing mode?

@martinwork
Copy link
Contributor

Flags for: C++/makecode; flashIncomplete; compass calibrated?
4 binary flags could be encoded as a hex digit: F rather than 1111.

Or is this sort of state info not appropriate in the model characteristic?

@martinwork
Copy link
Contributor

getMicrobitModel is declared in MicroBitBLEManager.h but not defined in MicroBitBLEManager.cpp.

MICROBIT_MODEL_XXX string macros are defined in MicroBitDevice.h but only used in MicroBit::getModel() in lancaster-university/microbit.

MicroBit::getModel() returns a string. Would it make sense to have a function that returns a simple version number for use in code and turn this into a string for calling MicroBitBLEManager::init? Could the string conversion avoid including more text constant instances containing "BBC micro:bit"?

It looks like MICROBIT_BLE_MODEL is no longer used.

@microbit-sam
Copy link
Contributor Author

Does this need to be implemented in DAL 2.1.0 for anything in iOS app @martinwork ?

It sounds like this should be reworked slightly and we should all agree on a format before this goes forward, but just that this is a "nice to have" feature rather than a requirement when 1.5 is out

  • 1.3x/1.5x are used rather than an integer to map directly to the board variant
  • Mode flags (also discussed in Determine over Bluetooth is micro:bit is in pairing mode #332) are an interesting idea but could a changing name confuse some users?
  • getMicrobitModel(): Ok, need to remove that
  • MICROBIT_MODEL_XXX string macros live in MicroBitDevice.h with all the other macros which makes sense to me
  • MicroBit::getModel() returns a string: You're right, a version number is probably more useful and should be implemented

@martinwork
Copy link
Contributor

The only use the app has for this is that if it sees "1.3", it disables the alert for old hex files, which tells the user they might need an update to work with their micro:bit. Without the version, the app will always alert for old hex files. I still think integer version numbers would be better, but I have assumed "1.3" in the app now, so let me know if you change it! Most users will not see the model string. It doesn't need to be confusing, if it is specified and documented that it could change, but I don't know if it can or should be changed.

@martinwork
Copy link
Contributor

whatAmI (maybe should be whoAmI) is implemented using a virtual function.

Would it use less RAM and be simpler to add a single byte member variable in MicroBitAccelerometer for the subclasses to store their ID, returned by a non-virtual function in MicroBitAccelerometer?

Maybe there are enough unused bits in MicroBitComponent::status?

@@ -150,7 +150,7 @@ class MicroBitBLEManager : MicroBitComponent
* bleManager.init(uBit.getName(), uBit.getSerial(), uBit.messageBus, true);
* @endcode
*/
void init(ManagedString deviceName, ManagedString serialNumber, EventModel &messageBus, bool enableBonding);
void init(ManagedString deviceName, ManagedString serialNumber, EventModel &messageBus, bool enableBonding, ManagedString microbitModel = ManagedString("BBC micro:bit"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to avoid using heap memory to store this info?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants