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

Support for Eddystone EID and other beacon layouts for Android. #45

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

stoneman1
Copy link
Collaborator

Sorry for long commit. Anyway here is Eddystone EID and other beacon layouts support for Android. Let me know what you think about this approach. I know this is going to be a breaking change, but I think this is better than unbindind and then again binding the beacon manager.

@MacKentoch
Copy link
Owner

MacKentoch commented Sep 20, 2017

Thank you @stoneman1

Just an amazing huge PR 👏

I will review later, but as a fast feedback I think I really like your approach 👍

@MacKentoch
Copy link
Owner

Trying to find a way to make things clear for a beacon new comer (React Native devs are more often just JS dev so I don't want them to be afraid of the library).
I mean binding is not something natural / easy to understand.

I'm asking below to think loud:

  • in fact, why would we need to unbind?
    If in need to change layout configuration I think about native removeParser method. We could offer JS side these simple methods:
      function addEddystoneUIDDetection(): void {
        BeaconsManager.addParser(EDDYSTONE_UID);
      }
    
      function removeEddystoneUIDDetection(): void {
        BeaconsManager.removeParser(EDDYSTONE_UID);
      }

@stoneman1
Copy link
Collaborator Author

Unbinding is needed if we want to support changing the Parsers during running. Altbeacon wants to have all the layouts before binding so adding Parsers did not work. This was the situation when I last time tried it.
This is the flow:

  1. Add layouts (eddystoneUID, iBeacon and whatever)
  2. Bind the manager
  3. Now it is scanning for all the layouts defined in step 1.
  4. If you try to add a new Parser here it won't start scanning them.
  5. Unbind
  6. Add layouts
  7. Bind again
  8. Now it is scanning those added layouts.

Hopefully that made any sense :)

@MacKentoch
Copy link
Owner

MacKentoch commented Oct 29, 2017

I adapted a bit your implementation to make it more talkative to dev (who may don't even know about beacons):

const { identifier, uuid } = this.state;
    // start iBeacon detection
    Beacons.addIBeaconsDetection()
    .then(() => Beacons.addEddystoneUIDDetection())
    .then(() => Beacons.addEddystoneURLDetection())
    .then(() => Beacons.addEddystoneTLMDetection())
    .then(() => Beacons.addAltBeaconsDetection())
    .then(() => Beacons.addEstimotesDetection()) 
    .then(() => {
      const region = { identifier, uuid }; // minor and major are null here

      Beacons
      .startRangingBeaconsInRegion(region) // or like  < v1.0.7: .startRangingBeaconsInRegion(identifier, uuid)
      .then(() => console.log('Beacons ranging started succesfully'))
      .catch(error => console.log(`Beacons ranging not started, error: ${error}`));

      Beacons
      .startMonitoringForRegion(region) // or like  < v1.0.7: .startRangingBeaconsInRegion(identifier, uuid)
      .then(() => console.log('Beacons monitoring started succesfully'))
      .catch(error => console.log(`Beacons monitoring not started, error: ${error}`));
    })
    .catch(error => console.log(`Beacons ranging not started, error: ${error}`));
   }

Async await way is even more talkative:

const { identifier, uuid } = this.state;
    try {
      await Beacons.addIBeaconsDetection();
      await Beacons.addEddystoneURLDetection(); 
      await Beacons.addEddystoneTLMDetection();
      await Beacons.addAltBeaconsDetection();
      await  Beacons.addEstimotesDetection();

      const region = { identifier, uuid }; // minor and major are null here

      await Beacons.startRangingBeaconsInRegion(region) // or like  < v1.0.7: .startRangingBeaconsInRegion(identifier, uuid)
      console.log('Beacons ranging started succesfully');
      
      await Beacons.startMonitoringForRegion(region) // or like  < v1.0.7: .startRangingBeaconsInRegion(identifier, uuid)
      console.log('Beacons monitoring started succesfully')
    } catch () {
      console.log(`Beacons ranging not started, error: ${error}`);
    }

@plrdev
Copy link

plrdev commented Oct 30, 2017

I think having higher level api like that can be useful to be easy to use, but it would still be good to have a lower level one as well, as @stoneman1 introduced here. They can coexist and higher level one can use the low level api, so no repetition needed either.

There are real use cases to wanting to change which layouts to listen etc, which the high level api don't allow to be implemented.

@MacKentoch
Copy link
Owner

MacKentoch commented Oct 30, 2017

I'm thinking about about adding a jack-of-all-trades case:

const parsers: Array<string> = [
 Beacon.PARSER_IBEACON,
 'SOME_CUSTOM_LAYOUT_WHY_NOT'
]; 

await Beacon.addParsersListToDetection(parsers)

It will avoid all these unbinding and binding for each parsers at initialization when we knwo exactly at once what parsers we need.

By the way, credits to @stoneman1 since all of that comes from your precious help.

@plrdev
Copy link

plrdev commented Oct 30, 2017

A fine idea, takes away the need to call bunch of layout adding methods. But still doesn't remove the possible use cases for lower level access, to change layouts once ranging/monitoring has been started.

@MacKentoch
Copy link
Owner

Working on removing 1 or more parsers.

I did not even provide an example since I want to find a nice solution.
removeParser function exists but I'm not sure it will be the told way to deal with.

@stoneman1
Copy link
Collaborator Author

Sorry been a bit busy. Lower level access would also be cool. I am working on iOS changes to get it to work with all beacons. Btw love your high level API 👍

@dukhanov
Copy link
Collaborator

dukhanov commented Mar 1, 2018

Hello,
any updates for this PR, do you have plans to merge it?

@maicWorkGithub
Copy link

_reactNativeBeaconManager.default.addIBeaconDetection is not a function

@jdegger
Copy link
Contributor

jdegger commented Sep 24, 2020

Why was this great PR never merged? 😢

Is this still - remotely - alive?

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

8 participants