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

Checking type size is not enough #67

Closed
bytesnake opened this issue Oct 4, 2022 · 5 comments
Closed

Checking type size is not enough #67

bytesnake opened this issue Oct 4, 2022 · 5 comments

Comments

@bytesnake
Copy link

There is currently a type size check at runtime, preventing out of array access for input consumers. https://docs.rs/futuresdr/latest/src/futuresdr/runtime/topology.rs.html#199

This does not prevent undefined behaviour, for example it allows to connect f64 to Complex<f32> arguments:

use futuresdr::anyhow::Result;
use futuresdr::blocks::NullSink;
use futuresdr::runtime::Flowgraph;
use futuresdr::runtime::{Block, Runtime};
use futuresdr::blocks;
use futuresdr::blocks::Fft;

/// Create sine tone.
pub struct Oscillator64;

impl Oscillator64 {
    pub fn new(freq: f64, amp: f64, sample_rate: f64) -> Block {
        let mut arg = 0.0;
        let diff = 2.0 * std::f64::consts::PI * freq / sample_rate;
        blocks::Source::new(move || {
            let s = amp * f64::sin(arg);
            arg += diff;
            arg = arg.rem_euclid(std::f64::consts::PI * 2.0);
            s
        })
    }
}

fn main() -> Result<()> {
    let mut fg = Flowgraph::new();

    let src = Oscillator64::new(440.0, 0.3, 48000.0);
    let fft = Fft::new(512);
    let snk = NullSink::<f64>::new();

    // uh oh \o/
    let src = fg.add_block(src);
    let fft = fg.add_block(fft);
    let snk = fg.add_block(snk);

    fg.connect_stream(src, "out", fft, "in")?;
    fg.connect_stream(fft, "out", snk, "in")?;

    Runtime::new().run(fg)?;

    Ok(())
}
@bastibl
Copy link
Member

bastibl commented Oct 4, 2022

Yes, this is true. There are, at least at the moment, many ways to shoot yourself in the foot, if you really want to. I spent quite some time to come up with an API that uses more strict typing, but it's really not as trivial as it may seem on first sight. If you have concrete suggests, your PR will be very much appreciated.

@bytesnake
Copy link
Author

no I just started to read the source and stumbled on https://github.com/FutureSDR/FutureSDR/blob/main/src/blocks/fft.rs#L104 which made me take note here. It would be great to at least check that all downstream node's scalar are compatible to the input scalar of current node.

@bastibl
Copy link
Member

bastibl commented Oct 4, 2022

You can cast the stream inputs and outputs to anything. That's not specific to this mutable reference to the input. It's the case for all stream connections.

A runtime check should be rather easy to implement by requiring any item type to implement Any and store the TypeId in the stream port. But that's just a runtime check... not sure if this is good enough for you.

@bytesnake
Copy link
Author

A runtime check should be rather easy to implement by requiring any item type to implement Any and store the TypeId in the stream port. But that's just a runtime check... not sure if this is good enough for you.

that would definitely be the reasonable way to go 👍

@bastibl
Copy link
Member

bastibl commented Oct 8, 2022

Implemented in ba57854.

Stream inputs/outputs are created with the actual type add_input::<T>("in")/add_output::<T>("out"). The actual ports remain non-generic, since only the any::TypeId of T is stored.

This TypeId is checked during connection and when casting the slice in work. There are also slice_unchecked versions, which ignore this check, since some blocks are, for example, easier to implement by casting it to a u8 slice in work.

@bastibl bastibl closed this as completed Oct 8, 2022
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

No branches or pull requests

2 participants