search

embassy-rs / nrf-softdevice

Apache License 2.0
266 stars 79 forks source link

Is there any plan of ANT+ support? #155

Closed sureshjoshi closed 1 year ago

sureshjoshi commented 1 year ago

Hi, apologies if this question doesn't make sense. I was recently introduced to Embassy, so I'm trying to wrap my head around this.

I have a project using the Garmin SoftDevice variant which supports ANT+, and I'm wondering if there is upcoming ANT+ binding support.

If not, does my requirement for ANT+ prevent me from using Embassy in my project?

Thanks!

Dirbaio commented 1 year ago

There's no plans for ANT support.

If the BLE API of ANT+BLE softdevices is the same, the BLE part should work with this crate. You could generate bindings for the ANT part and use them in parallel.

Also, we won't be able to carry support for the ant softdevices here if the headers aren't freely redistributable (don't know if they are, downloading requires login...)

sureshjoshi commented 1 year ago

Thanks!

I guess another semi-question should be, do I need to generate the bindings for this to work? Or could I just manually suffer the standard Rust/C interop on my own?

I'm just trying to determine what (if any) overhead I'll have testing this out.

sureshjoshi commented 1 year ago

Actually, I guess the question I just asked has roughly the same answer. :)

Dirbaio commented 1 year ago

the nrf-softdevice-gen tool should work, if you're lucky and the headers look the same (using SVCALL etc)

That generates just the low-level bindings (all unsafe).

sureshjoshi commented 1 year ago

Thanks! I'll report back in a couple weeks with any success/failures for the sake of future people in my boat

sureshjoshi commented 1 year ago

Turns out I never came back to detail this process. Putting this here for posterity (and honestly, for when I forget how to do it in a couple months).

MacOS aside

I continually ran into this error { kind: InvalidData, message: "stream did not contain valid UTF-8" } in nrf-softdevice-gen/src/main.rs. I found out the issue was the generator falling over when it encountered .DS_Store files on my Mac. I modified that main.rs file to skip over those.

S3xx additions

I use the S340, but change yours appropriately.

Modifications to nrf-softdevice

Note: I added an "ant" feature, but for simplicity, leaving that out.

Creating bindings

Note: I still haven't successfully run my application, as the stack re-direction to my application doesn't appear to work yet.

sureshjoshi commented 1 year ago

@Dirbaio I've generated the bindings as above, and all of my bluetooth code works, but I just noticed that I'm missing most (but not all) of the ANT+ APIs.

SVCALL(SVC_ANT_BURST_HANDLER_REQUEST, uint32_t, sd_ant_burst_handler_request(uint8_t ucChannel, uint16_t usSize, uint8_t *aucData, uint8_t ucBurstSegment)); is correctly bound.

However, just below it: SVCALL(SVC_ANT_BURST_HANDLER_WAIT_FLAG_DISABLE, uint32_t, sd_ant_burst_handler_wait_flag_disable (void)); doesn't show up in my bindings.

In fact, on further inspection - it looks like maybe 3 of the 30 APIs were bound, and the rest were ignored (no warnings, no errors).

Would you have any suggestions on how I could work on debugging this? Or if there are any conditions to what gets bound, and what isn't bound?

sureshjoshi commented 1 year ago

I should note that it appears all of the constants have been created:

pub const SVC_ANT_BURST_HANDLER_REQUEST: self::c_uint = 201;
...
pub unsafe fn sd_ant_burst_handler_request(ucChannel: u8, usSize: u16, aucData: *mut u8, ucBurstSegment: u8) -> u32 {
    let ret: u32;
    core::arch::asm!("svc 201",
...
}

I'm guessing this is something about the gen regex, as the first segment is grabbed perfectly.

sureshjoshi commented 1 year ago

Okay, think I figured it out. Once I knew it's probably a regex thing - I lined up some "good" and "bad" APIs together to see the difference.

Clear as day.

"good"
SVCALL(SVC_ANT_BURST_HANDLER_REQUEST, uint32_t, sd_ant_burst_handler_request(uint8_t ucChannel, uint16_t usSize, uint8_t *aucData, uint8_t ucBurstSegment));

"bad"
SVCALL(SVC_ANT_BURST_HANDLER_WAIT_FLAG_DISABLE, uint32_t, sd_ant_burst_handler_wait_flag_disable (void));

The "bad" ones have a space between the function name and the opening parentheses of the function params.

sureshjoshi commented 1 year ago

The fix appears to be to correct the regex to:

r"SVCALL\((?P<svc>[A-Za-z0-9_]+),\s*(?P<ret>[A-Za-z0-9_]+),\s*(?P<name>[A-Za-z0-9_]+)\s*\((?P<args>.*)\)\);",

After I the ANT+ up and running again, I'll make a PR for this.

Yandrik commented 1 year ago

I actually have a complete ANT implementation on my side already, with bindings generation, channels, networks and everything. It can even do all the ANT+ stuff such as setting a network key and such. It's not super comprehensive, but it works really well so far. Problem is Garmin's license agreement. Double-check that you can actually publish this. I don't think we're allowed to (at least the headers and bindings are off-limits). But I'd of course appreciate if you could look over the license again, and - if this is actually legal - we could collaborate on an ANT+ implementation for the crate.

sureshjoshi commented 1 year ago

@Yandrik Sorry, turns out I completely forgot about my PR and replying to this 🤦🏽

As far as I know, we can't publish it, which is lame. Have you bound your implementation using just the generated bindings? Or did you also wrap them in async calls?

Yandrik commented 1 year ago

The bindings are completely safe, and are made to be fairly similar to the original API. Here's a sample:

#[cortex_m_rt::entry]
fn main() -> ! {
    info!("nRF52832 ANT Master Channel Example");

    info!("Initializing Softdevice...");

    let conf = Config::default();
    let mut sd = Softdevice::enable(&conf, LicenseKey::EvaluationOnly);

    info!("Softdevice enabled");
    info!("Registering ANT Channel...");
    let chan_conf = AntChannelConfig {
        channel_num: 1,
        channel_type: AntChannelType::Master,
        ext_assign: AntExtAssign::new(),
        rf_freq: /* ANTPLUS FREQ */,
        transmission_type: AntTransmissionType::SharedChannel1byteAddress,
        device_type: DeviceType::new(1_u8.try_into().unwrap(), true),
        device_num: 0x1234_u16.into(),
        channel_period: 8192,
        network_number: 1,
    };

    let mut counter: u64 = 0;
    let mut counter_ref = &mut counter;

    let callback = |chan: &AntChannel, msg| {
            info!("handle: ANT Message: {}", msg);
            if let AntMessage(AntMessageType::Rx, Data(AntDataType::Broadcast, data)) = msg {
                info!("data is {}", data);
                *counter_ref += u64::from_le_bytes(data);
                chan.broadcast(counter_ref.to_le_bytes())
                    .expect("broadcast unsuccessful");
            }
        };

    sd.ant
        .create_channel(&chan_conf, callback)
        .expect("Couldn't create ANT Channel");

    /* block on */ sd.run_with_callback(|evt| info!("handle: SocEvent {:?}", evt));
}

This probably won't work perfectly, it's cobbled together as I stopped maintaining the examples a while ago, but it gets the point across.

All error types are as idiomatic as possible, and I've tried to reduce RawErrors as much as possible, or make them explainable. I also have hundreds of lines of documentation and such, which I'll hope will see the light of day at some point.

General idea is this:

sureshjoshi commented 1 year ago

Oh yeah, that's great!

Right now, I'm just using the generated bindings manually - it's not ideal, but more of a bandwidth issue as of today for me.

I'd need to look closer at the licensing, as re-reading it, it implies that the network key is really the only prohibiting factor. Hard to tell whether that's for "shared source code" vs "protocol stacks".

It would be great to have Rust bindings without the network key present.

Yandrik commented 1 year ago

That's totally possible. My Network Key Setting basically looks like this:

    pub fn set_network_address(&mut self, network_num: u8, network_key: &[u8; 8]) -> AntResult<()> {
        let ret = unsafe {
            raw::sd_ant_network_address_set(
                network_num,
                network_key as *const u8)
        };
        RawError::convert(ret)?;
        Ok(())
    }

So the network key itself wouldn't be in there, and does need to be set by the user (using sd.ant.set_network_address()). What would be in there would be the License Key. That's harder to remove, plus you literally can't get the evaluation license key anywhere but from the headers. Right now (as in the example) it's basically LicenseKey::EvaluationOnly or LicenseKey::Key("Some-License-Key"), but that's not very convenient to remove sadly. Possible, but it would make the library a hassle to use. Then again, you already have to download the Softdevices from Garmin anyways, so if the program crashes with a nice compile error (or the init has a nice docstring saying something like look at line XXX of the header nrf_sdm.h file), that might be acceptable... maybe?

sureshjoshi commented 1 year ago

Yeah, again, depends on how Garmin classifies the ANT bindings. Are they "protocol" (seems to need to be closed source), or "shared source" (can't show eval).

In the latter case, throwing an exception (ideally at compile time) is totally fine, as I think that's what the SDK itself does.

#error "You must obtain a valid license key to use ANT. You may use the evaluation key for non commercial use only by uncommenting it above this error. Commercial use license keys are available from ANT Wireless."
Yandrik commented 1 year ago

The problem with that is that the license key can only really be obtained by downloading the softdevice, and then manually sifting through the headers, copying the string out of there, and then using it in the init function. That works, probably, but is incredibly clunky. If I'm using the header-generated bindings already, the eval license key shouldn't be a big deal I think, especially because it's already out there in forum posts and such. But I think I should ask in the ANT+ forum and see if they have something against doing this. Publishing the bindings might be fine, but the license is properly opaque, and very all-rights-reserved. I'll look into getting clearance there once I have some more time on my hands.

sureshjoshi commented 1 year ago

Closing as there is no plans of ANT+ support in here due to possible/probably licensing concerns.

Can re-open as needed, or if there are any other useful code snippets.

sureshjoshi commented 1 year ago

One final thought on this - if someone maybe has an opinion. If we avoid entirely the ANT+ source code, generated bindings, and license key - I can't imagine there is a problem with creating the "sugar'd" ANT+ code, feature'd out of compilation.

For those who want to use it, instructions on where to put their source code + network key, and then run the generator, so the actual Garmin source code only ever exists on those people's machines, and never GitHub.

So: