e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

Plumb through new "matched bluetooth vehicle" from the phone to the server and to the trip #1062

Closed shankari closed 7 months ago

shankari commented 8 months ago

@JGreenlee @louisg1337 @the-bay-kay for visibility and feedback

shankari commented 8 months ago

We need to make some design decisions before implementing this feature.

Decision 1: Do we need to store this as a separate object, or should we treat it as a statemachine transition? When we use the iOS visit API, we generate VISIT_STARTED and VISIT_ENDED transitions in the current state. Given that our primary use case here is to use the beacon region enter and exit to detect the start and stop of fleet vehicles, can we just use similar BEACON_STARTED and BEACON_ENDED messages?

I think that is not the best option. Some challenges with that approach:

The third is not a great argument, since the visit transitions may also not trigger an FSM transition if the geofence has already triggered. But I think that the second is really compelling. So let's go with a separate object.

Decision 2: Should the separate object be a manual/ or a background/? Per the data model, I think it should be a background/ object. Although it is not necessarily being continuously sensed now, there is no manual intervention required to generate it. And in the future, I guess we could also periodically monitor the signal strength during the course of the trip to ensure that the enter was not an outlier, or with Bluetooth classic across phones, etc.

Decision 3: What should the data structure look like? Here's a proposal based on copying the BLE beacon + classic structure over and combining it with vehicle information. @JGreenlee, is this consistent with what you had in mind for the values in the dynamic config?

{
    bluetoothType: CLASSIC | BLE
    bluetoothDetails: BluetoothClassicDetails | BLEDetails
    vehicleType: CAR | BUS | BICYCLE | ...
    vehicleDetails: CarDetails (can potentially include BikeDetails | BusDetails | .... in the future as we expand)
}

BluetoothClassicDetails (from https://developer.apple.com/documentation/externalaccessory/eaaccessory?language=objc)
The original spec also has firmware revision and hardware revision. Not sure how many of those incidental fields are required for our use case. Need to experiment and tweak
{
    name:
    manufacturer:
    modelNumber:
    serialNumber:
    protocolStrings:
    connectionID:
}

BLEDetails (basically, region and event type, I am also supporting range although it is not clear if we will need to use it
{
    uuid:
    major:
    minor:
    proximity: UNKNOWN | IMMEDIATE | NEAR | FAR
    rssi:
    tx:
    accuracy:
}

vehicleDetails
{
    license:
    make:
    model:
    year:
    engine: ICE | HYBRID | BEV | HYDROGENV | BIOV
}
JGreenlee commented 8 months ago

For decision 3, would this be the structure of the stored background/ object?

I thought this would only include the bluetooth beacon information.

And I thought the vehicle information would be in the dynamic config. Something like this:

{
  ...
  "vehicle_identities": [
    {
      "value": "e_car_1234",
      "bluetooth_ids": ["00:1A:2B:3C:4D:5E"],
      "name": "Leaf A",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "make": "Nissan",
        "model": "Leaf",
        "year": 2018,
        "license": "AAA 1234"
      }
    },
    {
      "value": "e_car_5678",
      "bluetooth_ids": ["00:1A:2B:3C:4D:5F"],
      "name": "Leaf B",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "make": "Nissan",
        "model": "Leaf",
        "year": 2020,
        "license": "BBB 5678"
      }
    }
  ],
  ...
}
JGreenlee commented 8 months ago

If we are going to support BLE and Bluetooth classic (maybe in the future), and these have different properties, what about having 2 different types: background/bluetooth_ble and background/bluetooth_classic

shankari commented 8 months ago

For decision 3, would this be the structure of the stored background/ object? Yes.

I thought this would only include the bluetooth beacon information. And I thought the vehicle information would be in the dynamic config. Something like this:

That could also work. Per my conversation with @the-bay-kay, since we have to have the full list of UUIDs to "register" them, it would be easy enough to have all the information in the background/ object as well. Is that correct, @louisg1337?

Let us think through the various options and their pros and cons.

Must happen native code must receive a list of BLE beacons so that it can create the appropriate "regions". The list must come from the dynamic config so that it can be different for each fleet. Question 1: How are the beacons configured:

  1. UI reads dynamic config and passes it in to native code via a function call a. UI only passes in BLE UUIDs b. UI passes in full vehicle_identifier object
  2. The dynamic config is saved into local storage as usual (unchanged)
    • The native code reads the dynamic config directly from native config and configures the regions

Question 2: What do the background objects look like?

  1. Native code generates background/ objects with only beacon UUID
    • When UI displays "draft" trips, it reads the background/ objects, maps them to the dynamic config, and creates the draft composite trips with the vehicle information filled in
      • On the server, we read the dynamic config match the background/ objects to sections and copy them into the confirmed and composite objects. Note that reading the dynamic config on the server is relatively new.
        1. Native code generates background/ objects with beacon UUID + whole vehicle information
      • When UI displays "draft" trips, it reads the background/ objects and displays them
      • On the server, we match the background/ objects to sections and copy them (no need to read and handle the dynamic config)

For the decisions: Q1: I think (2) is better because I am not sure how long the regions persist and when they are deleted. On android, for example, IIRC that all location-based geofences are deleted when the app is updated. If we have the native code read the values and register directly, we can do so even if the user hasn't launched the app. Q2: I don't have a strong preference. (2) is a bit easier to implement, but will take up a little more storage.

@JGreenlee do you have a preference?

shankari commented 8 months ago

Also, I had a question around must happen (aka native code must receive a list of BLE beacons so that it can create the appropriate "regions"). From the iOS docs, it looks like we can have multiple beacons with the same UUID and different major and minor values https://developer.apple.com/documentation/corelocation/determining_the_proximity_to_an_ibeacon_device?language=objc

The UUID (universally unique identifier) is a 128-bit value that uniquely identifies your app’s beacons. The major value is a 16-bit unsigned integer that you use to differentiate groups of beacons with the same UUID.

...

The most important point to me here is the part which says "your app’s beacons". Since the beacons are really intended for use with OpenPATH, maybe we can configure them all with the same UUID, that is hardcoded (or put into an app-level config) into OpenPATH. Then we only need to listen for one UUID, note that the major and minor values can be wildcards in the scan. This might also be good for long-term scalability, so that we don't run into the maximum number of geofences we can create. At least for location-based regions on iOS, this limit seems to be 20.

Regions are shared resources that rely on specific hardware capabilities. To ensure that all apps can participate in region monitoring, Core Location prevents any single app from monitoring more than 20 regions simultaneously. To work around this limitation, monitor only regions that are close to the user’s current location. As the user moves, update the list based on the user’s new location.

I did not find a similar number of beacon-based regions, but the message above says "Core Location" and this SO post claims that the limit also applies to beacon regions https://stackoverflow.com/questions/8734101/what-is-the-maximum-number-of-regions-that-can-be-monitored-with-startmonitoring#comment52676399_8734329 (20 for location, 20 for beacons)

Further, the example for how to use the beacons https://developer.apple.com/documentation/corelocation/determining_the_proximity_to_an_ibeacon_device?language=objc makes it very clear that the UUID and the identifier are expected to be fixed on a per-app basis.

@louisg1337, is it possible for us to experiment with multiple beacons configured with the same UUID? If we leave the major and minor keys as blank while registering the region, does the callback return them? Or do we have to start scanning to get that information? It is fairly clear that the major and minor values will show up if we start scanning.

We can't test this with the current UI since the hardcoded values have the major and minor key defined. Fortunately I made a change last night that allows us to put new entries into the list. Or of course, you can just edit the current app to remove major and minor keys.

We need to answer this first thing because it will affect everything downstream

If we are going to search for a hardcoded (or app-level configured) UUID, then the native code does not need a list of UUIDs from the dynamic config, which means that we have a strong vote for the second option for Question 2.

JGreenlee commented 8 months ago

@JGreenlee do you have a preference?

After seeing the approaches for Q2 laid out, I don't have a strong preference either.

One reason I might express a slight preference for (2) is that it allows the native code to be agnostic of vehicles. It would only have to worry about beacons, so the native code would be a bit simpler and more generic.

On the server, we read the dynamic config match the background/ objects to sections and copy them into the confirmed and composite objects. Note that reading the dynamic config on the server is relatively new.

Would we necessarily have to do this? What if the trip object just had value?. This would be similar to what we do for mode+purpose labels; the trip object just has the string value for the label and the UI looks up baseMode, met, and co2PerKm from the label_options config.

shankari commented 8 months ago
Added two, about to add third entry After adding the third entry
simulator_screenshot_A54968C6-320E-4943-828F-E75724152532 simulator_screenshot_46DCCD1F-D3A9-49D9-98FF-DB0055DA3588
shankari commented 8 months ago

The plugin that we use requires both an identifier and a UUID. This is because the call to create a beacon used to require both a UUID and an identifier. https://developer.apple.com/documentation/corelocation/clbeaconregion/3183025-initwithuuid?language=objc

However, that call is now deprecated. We should not be using CLBeaconRegion anymore, we should be using CLBeaconIdentityCondition. This new class only requires a UUID. It has no reference to an identifier.

Even in the old days, the identifier was static and hardcoded

        // Match all beacons with the specified UUID
        let proximityUUID = UUID(uuidString: 
               "39ED98FF-2900-441A-802F-9C398FC199D2")
        let beaconID = "com.example.myBeaconRegion"

        // Create the region and begin monitoring it.
        let region = CLBeaconRegion(proximityUUID: proximityUUID!,
               identifier: beaconID)

Note also that the UUID in the new method is of type NSUUID, which is a 128-bit value created to conform to RFC 4122. It cannot be an arbitrary string. So from the example above, we must use "39ED98FF..." and we cannot use "com.example.myBeaconRegion"

Since the identifier is still required in the plugin, I am going to use a hardcoded value for it, edu.berkeley.eecs.emission to be consistent with the example.

Note also that the way to avoid specifying the major and minor values is to use undefined https://github.com/louisg1337/cordova-plugin-ibeacon?tab=readme-ov-file#specify-wildcard-uuid-android-only

shankari commented 8 months ago
Optional major and minor Refactored display to highlight UUID and major/minor
before_ui_changes simulator_screenshot_F50E191C-D5D8-46C9-8702-E8EFC985784A
shankari commented 8 months ago

I am working on a PR to simulate BLE scans https://github.com/e-mission/e-mission-phone/pull/1140

shankari commented 8 months ago

It's going to be a bit tricky to know how to display the matches to see if we have minor and major values without running the code, since there is not much documentation (from the iOS level up) around what this may look like. Let's just stringify the result and display it for now.

louisg1337 commented 8 months ago

@louisg1337, is it possible for us to experiment with multiple beacons configured with the same UUID? If we leave the major and minor keys as blank while registering the region, does the callback return them? Or do we have to start scanning to get that information? It is fairly clear that the major and minor values will show up if we start scanning.

I'm not quite sure, but I can definitely test this out especailly since I have 2 beacons now. I also really do like this idea of using one UUID and variable major/minor values a lot better than creating X amount of regions. The latter seems a bit wasteful resource wise, and it also complicates things quite a bit.

Jack also made a great point here how we may need to change up our implementation a bit, but it doesn't seem like too crazy of a pivot. I'll give all of this a go and update accordingly as I get some answers!

louisg1337 commented 8 months ago

Quick follow up to my comment above, I did two tests.

  1. Provided only the UUID and attempted to monitor for beacons entering our region. When we detect a beacon in the region, we get a callback with int state, Region region as our parameters. I figured maybe the region could maybe have the major and minor values of the beacons it detected in it. I set my accent beacon to have the same UUID as my Blue Charm one, and when I monitored the region it detected two beacons, but whenever I tried to return the major and minor values using region.toString() (API docs, it shows up as null. Jack hit the nail on the head with that one, the region variable is just the region that we define.

  2. I then tried the ranging functionality as Jack suggested instead while providing only the UUID. This one seems like it works! Upond ranging we get Collection<Beacons> beacons as a parameter, and each value in beacons carries its major and minor value, despite us not providing it in the region.

With that in mind, looks like a fixed UUID and variable major/minor values could be the way to go!

shankari commented 8 months ago
Just started scan fake callback for existing beacon Add new beacon with no major or minor Fake callback for new beacon
simulator_screenshot_2237CD71-BE67-4243-8D37-63D415C9BDC8 simulator_screenshot_87E3A742-A796-4571-BF9E-19B9732A2AAC simulator_screenshot_C64214D5-9BD9-4368-AC6C-85B84E075D4A simulator_screenshot_C2BC9C98-60BD-4D50-935C-1823962F03C6
shankari commented 8 months ago

Given these results, I think that the answers to the questions above are: ~Must happen native code must receive a list of BLE beacons so that it can create the appropriate "regions". The list must come from the dynamic config so that it can be different for each fleet.~

We can have a single hardcoded value for e-mission/OpenPATH

~Question 1: How are the beacons configured:~ This is N/A since the beacon UUID will be hardcoded

Question 2: What do the background objects look like?

@JGreenlee you also need to change the dynamic config to have major/minor values instead of the bluetooth_ids, but I don't think we will need to change a lot else. You might also want to think about how we represent the info for other vehicles (e.g. buses, trains and bikes). Trains will not have a license number and bikes may not. And buses will have a route, which is likely very important information. We don't need to fully design these now, but we need to make sure that we can support other types of vehicle information in the future without having to redesign everything. Maybe a vehicle_type to go with the vehicle_info?

JGreenlee commented 8 months ago

Here is an updated config proposal.

I changed bluetooth_ids to bluetooth_major_minor. It is in the format major:minor, each as 4 characters hexadecimal. It's probably good practice for each deployment to have a designated major that is used for all beacons in that deployment. in this example I went with dfc0

I added type and engine in vehicle_info.


{
  ...
  "vehicle_identities": [
    {
      "value": "e_car_1234",
      "bluetooth_major_minor": ["dfc0:1234"],
      "name": "Leaf A",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "type": "car",
        "make": "Nissan",
        "model": "Leaf",
        "engine": "BEV",
        "year": 2018,
        "license": "AAA 1234"
      }
    },
    {
      "value": "e_car_5678",
      "bluetooth_major_minor": ["dfc0:5678"],
      "name": "Leaf B",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "type": "car",
        "make": "Nissan",
        "model": "Leaf",
        "engine": "BEV",
        "year": 2020,
        "license": "BBB 5678"
      }
    }
  ],
  ...
}
shankari commented 8 months ago

in this example I went with dfc0

It is so cool that this deployment name lent itself so well to hex 😄 This looks good to me. I don't know if we will need the engine type, but I suspect it will be easier to remove than to add.

shankari commented 8 months ago

To pick this up again, with the proposed changes, the new background object will have a key of background/bluetooth_ble and a value of

{
    "ts": // always present
    "eventType": REGION_ENTER | REGION_EXIT | RANGE_UPDATE
     "uuid": // will always be present
     "major": // for our use case, missing for REGION_ENTER or REGION_EXIT
     "minor": // for our use case, missing for REGION_ENTER or REGION_EXIT
     "proximity": // only available for RANGE_UPDATE
     "rssi": // only available for RANGE_UPDATE
     "accuracy": // only available for RANGE_UPDATE
}

While creating raw sections, we will use the values for section segmentation. Exact algorithm TBD, but at a high level, if we see a change in BLE beacons, it is likely a change from one vehicle to another or from a vehicle to walking.

Design decision: should we fill in the vehicle values right here, or fill in the major:minor key here and fill in the full value for the cleaned section? It doesn't make a huge difference. Let's start with filling in the major:minor key in the segment_current_sections stage. Note that these can be filled in directly from the background object and do not need to read the dynamic config. Then, in the clean_and_resample stage, we can fill in the values from the dynamic config by simply copying the matching value in. This will allow the UI to read the baseMode, met_equivalent and kgCo2PerKm as needed.

@JGreenlee As an aside, I think we should standardize on camelCase or under_score_case. Right now, baseMode and kgCo2PerKm are in camelCase but met_equivalent is in under_score_case.

I think we should also have a summary of the values in the confirmed and composite trips, similar to the current inferred_section_summary and cleaned_section_summary to make it easier to find the primary mode instead of having to iterate over the sections. This could be called ble_sensed_section_summary and the keys would be the baseMode @JGreenlee is this consistent with what you had in mind?

I think that is about it. I am now starting with adding the new data type and plumbing it through to the server. Since we do not generate these objects in the native code yet, I will simulate them using the UI. After that is done, I just need to figure out an initial algorithm for segment_current_sections and we are done!

There will be a ton of tuning that will need to happen with real world testing next month to address all the weird corner cases that will undoubtedly show up.

shankari commented 8 months ago

Design decision These feels foundational enough that I am going to create a new data type instead of storing it as JSON. This will allow us to read the fields from the object directly instead of parsing through a JSON object and dealing with exceptions. We will use the object in the FSM, so this seems fairly important. https://github.com/e-mission/e-mission-docs/blob/master/docs/dev/back/adding_a_new_data_type.md

JGreenlee commented 8 months ago

@JGreenlee As an aside, I think we should standardize on camelCase or under_score_case. Right now, baseMode and kgCo2PerKm are in camelCase but met_equivalent is in under_score_case

Ah, I did it that way to match the fields in the label options spec.

https://github.com/e-mission/nrel-openpath-deploy-configs/blob/main/label_options/example-study-label-options.json It has the same inconsistency where met_equivalent is "snake_case" while baseMode and kgCo2PerKm are "camelCase".

That inconsistency has been around for a while, even before label options were customizable.

There are also more cases of this elsewhere in the dynamic config, and some places where a third one, "kebab-case" is used.

I don't know an easy way to fix it. I think we would have to implement a temporary backwards compat patch on the phone to convert local configs to one standard case, regardless of what case the source used. Then we could update all the configs on GitHub to use the standard case. And remove the temporary patch after we're sure everyone has gotten it.

For now, I think we should just decide what 'case' we want to end up with and use that for new fields going forward. I'd vote for "snake_case" since the e-mission platform is primarily a Python ecosystem.

So for the vehicle_identities spec we would use base_mode and kg_co2_per_km.

shankari commented 8 months ago

Ah, I did it that way to match the fields in the label options spec.

Ah, that is a good enough reason then. Note that the label options are also read in python now, to support custom labels in the public dashboard.

Let's keep it unchanged for historical reasons and deal with unifying it later. Much later. When the rest of the code is so polished that we have nothing else to polish.

JGreenlee commented 7 months ago

I think there should be an additional field in the config spec to differentiate programs that will only track Bluetooth-sensed trips vs. programs that will still track all trips and just include Bluetooth sensing as a supplement to the existing tracking.

The "fleet" version of OpenPATH is first scenario, but I think we want to leave the door open for the second scenario too.

{
  ...
  "tracking": {
    "bluetooth_only": true
  },
  ...
}

In total,


{
  ...
  "vehicle_identities": [
    {
      "value": "e_car_1234",
      "bluetooth_major_minor": ["dfc0:1234"],
      "name": "Leaf A",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "type": "car",
        "make": "Nissan",
        "model": "Leaf",
        "engine": "BEV",
        "year": 2018,
        "license": "AAA 1234"
      }
    },
    {
      "value": "e_car_5678",
      "bluetooth_major_minor": ["dfc0:5678"],
      "name": "Leaf B",
      "baseMode":"E_CAR",
      "met_equivalent":"IN_VEHICLE",
      "kgCo2PerKm": 0.08216,
      "vehicle_info": {
        "type": "car",
        "make": "Nissan",
        "model": "Leaf",
        "engine": "BEV",
        "year": 2020,
        "license": "BBB 5678"
      }
    }
  ],
  "tracking": {
    "bluetooth_only": true
  },
  ...
}
shankari commented 7 months ago

I have now created separate buttons for simulating bluetooth transitions. I now have a quick set of design decisions to make. How do I generate and save the values?

There are a couple of options:

  1. when the buttons are clicked in the card, directly save the entry and generate the transition. This means that the callbacks in the bluetooth scan page will only work with real beacons. It also means that the real beacon callbacks will only be displayed in the UI and not stored.
  2. when the buttons are clicked in the card, continue calling the methods in the javascript delegate. This method will now save the entry and generate transitions. This will ensure that when we have real beacon callbacks, they will also be stored correctly in the database.

I am currently leaning towards (2) because:

shankari commented 7 months ago

So far, I have primarily been testing this on iOS. I tested on android before pushing the changes to simulate the data storage and transitions, and found a few differences. I have accounted for some of the differences in the code, but we may need to either unify the plugin or do some additional cleanup later.

JGreenlee commented 7 months ago

@shankari In the background/bluetooth_ble object, how are major and minor represented?

Are they i) decimal integers or ii) strings of hexadecimal digits ?

shankari commented 7 months ago

@JGreenlee decimal integers

The raw iOS values return NSNumber, which is a char, short int, int, long int, long long int, float, or double or as a BOOL I am pretty sure that alt-beacon will be consistent with iOS because that's what they focus on after all

shankari commented 7 months ago

Trying out the native wrapper classes with a hack

    if ([transition isEqualToString:CFCTransitionBeaconFound]) {
        BluetoothBLE* enterWrapper = [BluetoothBLE new];
        enterWrapper.eventType = @"REGION_ENTER";
        enterWrapper.ts = [BuiltinUserCache getCurrentTimeSecs];
        [[BuiltinUserCache database] putSensorData:@"key.usercache.bluetooth_ble" value:enterWrapper];

        for (int i = 0; i < 5; i++) {
            BluetoothBLE* rangeWrapper = [BluetoothBLE new];
            rangeWrapper.eventType = @"RANGE_UPDATE";
            rangeWrapper.ts = [BuiltinUserCache getCurrentTimeSecs];
            [[BuiltinUserCache database] putSensorData:@"key.usercache.bluetooth_ble" value:rangeWrapper];
        }
        NSArray* currentEntries = [[BuiltinUserCache database] getLastSensorData:@"key.usercache.bluetooth_ble" nEntries:2 wrapperClass:StatsEvent.class];
        NSLog(@"[BLE native] Found %lu entries", currentEntries.count);
        // NSLog(@"[BLE native] First entry is %@, last entry is %@", currentEntries[0], currentEntries[currentEntries.count-1]);
        NSLog(@"[BLE native] while handling transition %@", transition);
     }

and am running into some very weird behavior.

The first save (of enterWrapper) succeeds, but the save of the rangeWrapper and the reads generate a bad access exception. Not immediately, but when the related object is garbage collected. So for rangeWrapper, the crash happens when we exit the first iteration of the for loop. For the read, the error happens when we exit handleAction, unless we comment in the line to access the first element, in which case it crashes then

shankari commented 7 months ago

Removing the for loop, we crash while exiting the if check Adding two more unrolled calls, can confirm that we go through them and then crash while exiting the ifcheck

        BluetoothBLE* rangeWrapper1 = [BluetoothBLE new];
        rangeWrapper1.eventType = @"RANGE_UPDATE";
        rangeWrapper1.ts = [BuiltinUserCache getCurrentTimeSecs];
        [[BuiltinUserCache database] putSensorData:@"key.usercache.bluetooth_ble" value:rangeWrapper1];

        BluetoothBLE* rangeWrapper2 = [BluetoothBLE new];
        rangeWrapper2.eventType = @"RANGE_UPDATE";
        rangeWrapper2.ts = [BuiltinUserCache getCurrentTimeSecs];
        [[BuiltinUserCache database] putSensorData:@"key.usercache.bluetooth_ble" value:rangeWrapper2];

I bet this is because of the inheritance from CLBeacon. It is a nice trick, but several of the properties are read-only, so we can't set them, and I bet they are uninitialized and crashing when we free the related memory. I vaguely remember something like this happening with the location wrapper; that is why SimpleLocation does not inherit from CLLocation

shankari commented 7 months ago

Confirmed that just after changing @interface BluetoothBLE : CLBeacon to @interface BluetoothBLE : NSObject all the access errors went away. Restoring the code and testing again...

Also need to convert the UUID to a string instead of NSUUID so that it can be serialized. And change the field name to uuid instead of UUID

shankari commented 7 months ago

I think that the plumbing support works fairly well, so I'm going to review and try to merge at least the android implementation. We should be able to test that using the UI simulator as well. After I am done with that, I can try to merge the phone display changes and,if it works, potentially push out to staging.

shankari commented 7 months ago

One challenge with merging in the android changes (and potentially the iOS changes down the road), is that the transition names are different. We should merge to a separate branch so that we can pick unified names and test before merging. Also, at least the android version is not saving anything; we will need to fix that before the UI can work.

The UI seems fairly basic and straightforward and supports multiple matching beacons. It just picks the beacon that occurs most frequently, so that should not be too bad.

shankari commented 7 months ago

Starting with the android changes: here are the new transitions added:

    <string name="transition_checking_for_beacon">local.transition.checking_for_beacon</string>
    <string name="transition_beacon_found">local.transition.beacon_found</string>
    <string name="transition_beacon_not_found">local.transition.beacon_not_found</string>

The middle one maps nicely to our ble_beacon_found. We should probably add ble_beacon_not_found to everything. Do we need checking_for_beacon? That sounds almost like a state and not a transition.

I don't think we need a separate transition for checking_for_beacon. If we find it, we just start the service. We can do that just as well from the handleTripStart state

        if (actionString.equals(ctxt.getString(R.string.transition_checking_for_beacon))) {
            Log.d(this, TAG, "Checking for beacons!");
            // Start up the bluetooth service to check for beacons
            Intent foregroundStartBluetooth = new Intent(ctxt, TripDiaryStateMachineForegroundService.class);
            foregroundStartBluetooth.setAction("foreground_start_bluetooth");
            ctxt.startService(foregroundStartBluetooth);

And with that, I don't think we actually even need beacon_not_found. If we don't get the beacon_found transition, we will just stay in waiting_for_trip_start.

So the changes needed for the android version are:

shankari commented 7 months ago

While saving the values, I noticed that the alt-beacon values are not a direct match to the ibeacon values. So we fall back to the standalone cordova plugin https://github.com/petermetz/cordova-plugin-ibeacon to figure out how to interpret the values correctly

Current assumptions are:

other values are copied over directly. I do not have a beacon yet (will order one tomorrow) so I don't have a way to test this. But I will at least make sure that it compiles before deploying to staging for others to test.

shankari commented 7 months ago

After my fixes to https://github.com/e-mission/e-mission-data-collection/pull/219 I believe that the android side should work with demo quality. @JGreenlee if you pull the data collection plugin from the integrate_ble branch, and the server changes from https://github.com/e-mission/e-mission-server/pull/963 you should be able to test against a real device. I have looked at the iOS changes and they don't appear to be very tricky. I might be able to finish them tomorrow and then push out a release to staging.

shankari commented 7 months ago

Per @the-bay-kay, things are not crashing. Going to merge this set of changes and get it on to staging to test. Can do another release tomorrow morning with UI changes (if any) since we can at least get the native code changes shaken out a bit before pushing to production.

shankari commented 7 months ago

The plumbing seems to work well enough for an "alpha" version. We may need to make changes as we polish, but closing this issue for now.