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

- update AltBeacon lib for bugfix 'No Monitoring information after ki… #443

Closed
wants to merge 1 commit into from

Conversation

rdehuyss
Copy link

  • update AltBeacon lib for bugfix 'No Monitoring information after killing an application built with the minify mode'
  • Set Waypoint in red when triggered by IBeacon

…lling an application built with the minify mode'

- Set Waypoint in red when triggered by IBeacon
@binarybucks
Copy link

What was the reason to change the Paho version back to 1.1.0?

@rdehuyss
Copy link
Author

The version mentioned in build.gradle was not available in JCenter, so I did not know on how to get it... are there any special bugfixes in it?

Here is the link for JCenter: https://jcenter.bintray.com/org/eclipse/paho/org.eclipse.paho.client.mqttv3/

@binarybucks
Copy link

Yes, 1.1.0 is pretty old and has several websocket related issues. Hence I'm using the snapshot builds directly from the Paho repository. They tend to get swapped out after new daily builds are uploaded so we've to change the dependency every couply of weeks. It probably will be some time until there is a stable 1.1.1 build anywhere.

I'll fix it and replace it with the current proper build.

@rdehuyss
Copy link
Author

rdehuyss commented Jan 19, 2017

Ok, I did not know that... hopefully the rest is ok.

There is one thing of which I think that it would be a nice improvement which is the following: if you now add a region which is IBeacon enabled, the BeaconManager is not restarted. Once the ServiceBeacon is instantiated (e.g. after a fresh install), it will not start the BeaconManager if you create a region.
Would that not be solved by Subscribing with the GreenRobot EventBus on the following method?

public void onEvent(Events.WaypointAdded e) { ... }

I did not add it because there might be a reason for it that I do not know.

Nice readable code by the way! Any interest in unit tests?

@binarybucks
Copy link

Yes, onEvent is fine, you can look at the ServiceLocator, it uses the same event to reinitialize regions.

I'm currently looking at the setType call. You're kind of misusing it but it is currently not used anywhere.
Also, what happens if a leave is missed (e.g. Bluetooth switched off while leaving). It would still show as inside, right?

Unit tests would be great, but so far there were more important things to fix ;)

@binarybucks
Copy link

binarybucks commented Jan 19, 2017

What about dropping wifiSSID, and type and adding a lastTriggeredState int. This way we could also add the inside/outside feature to location based waypoints.

In a second step we could then show them on the map with their according state which is an often requested feature.

This would require a db migration though (which should better not drop/recreate the db like in the last update).

@rdehuyss
Copy link
Author

  • Since it's not my codebase I was looking for the thing with the least amount of impact, so that is why I used the type which was indeed not used anywhere
  • Regarding leaving with bluetooth off AND not having a radius: I think you are right, it would still show as inside. But is that not a strange usecase? One way to solve it, would be to listen for changes on the bluetooth adapter. For me, showing the region in red was just to see whether I configured owntracks correctly for IBeacon support.
  • The idea of a lastTriggeredState sounds great - I just don't know how this would work with backwards compatability in the database...

Until the end of this month, I'm quite busy but once February I would be glad to help with these improvements...

@binarybucks
Copy link

Valid point, you can't get around it once you track state. If you miss a change, the state will be wrong.

I'll add another DB field for the state once @nma83 sends his widget PR. I don't want him to fix yet another round of merge conflicts :D
I've to add a db migration function anyway so we don't nuke waypoints on every db change.

@binarybucks binarybucks mentioned this pull request Jan 20, 2017
@binarybucks
Copy link

beacon updates were included manually

@binarybucks binarybucks closed this Apr 2, 2017
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.

None yet

3 participants