INTO-CPS-Association / unifmu

A universal mechanism for implementing Functional Mock-up Units (FMUs) in various languages
37 stars 4 forks source link

Problem with FMI3 tests on Linux #63

Open sagilar opened 8 hours ago

sagilar commented 8 hours ago

When executing cargo test on Linux in the branch dev/claudio, I get this error:

error[E0308]: mismatched types
   --> cli/tests/cli_tests_fmi3.rs:427:61
    |
427 | ...ce.get_interval_decimal(&[*vr], &mut interval, &mut qualifier).ok()....
    |       --------------------                        ^^^^^^^^^^^^^^ expected `&mut [u32]`, found `&mut [i32; 1]`
    |       |
    |       arguments to this method are incorrect
    |
    = note: expected mutable reference `&mut [u32]`
               found mutable reference `&mut [i32; 1]`

The original function looks this way:

fn get_interval_decimal(import: &Fmi3Import, cs_instance: &mut InstanceCS, vr: &u32) -> (f64, i32) {
    let mut interval: [f64; 1] = [0.0];
    let mut qualifier: [i32; 1] = [0];
    let error_msg = format!("get_interval_decimal failed for {}", vr);
    cs_instance.get_interval_decimal(&[*vr], &mut interval, &mut qualifier).ok().expect(&error_msg);

    (interval[0], qualifier[0])
}

The problem seems to be resolved by changing the data type of qualifier to u32 in line 425 and casting the return result to i32 in line 429 as follows:

fn get_interval_decimal(import: &Fmi3Import, cs_instance: &mut InstanceCS, vr: &u32) -> (f64, i32) {
    let mut interval: [f64; 1] = [0.0];
    let mut qualifier: [u32; 1] = [0]; // changed to u32 here
    let error_msg = format!("get_interval_decimal failed for {}", vr);
    cs_instance.get_interval_decimal(&[*vr], &mut interval, &mut qualifier).ok().expect(&error_msg);

    (interval[0], qualifier[0] as i32) // casting to i32 here
}

@clagms also got the same problem when running cargo test on Docker.

clagms commented 8 hours ago

@YonVanom to reproduce the problem just run the dockerized build. I think it's caused by us being too strict on the types for the FMI functions... that's just a guess...

YonVanom commented 8 hours ago

This is a known issue related to the use of the rust-fmi library. It was previously encountered by Aleksander. Here is some information from our discussion:

"As far as I understand, in C, the underlying datatype for an enum is typically an int, and should be an int32 on most systems. It’s only if there are too many enum values that the compiler can decide to make it an uint.

So as far as I understand, int32 should be correct."

"The rust-fmi library generates rust bindings from the standard fmi headers. It seems there is a difference when building for linux (in the docker) or for windows (local).

On windows I get this:

pub type fmi3IntervalQualifier = ::std::os::raw::c_int;

For linux (in the docker), I get this:

pub type fmi3IntervalQualifier = ::std::os::raw::c_uint;

The same is true for fmi3Status and fmi3DependencyKind, and also for the fmi2 bindings for fmi2Status, fmi2Type, and fmi2StatusKind."

"Seems to be a known “issue” when using bindgen. For OSX it will likely also result in c_uint: https://github.com/rust-lang/rust-bindgen/issues/1966 https://github.com/rust-lang/rust-bindgen/issues/1907

Still not sure what to do with this. We can use conditional compilation in cli_tests to handle this difference between platforms, e.g.:

#[cfg(target_os = "windows")]
pub type FmiIntervalQualifier = ::std::os::raw::c_int;

#[cfg(target_os = "linux")]
pub type FmiIntervalQualifier = ::std::os::raw::c_uint;

However, this raises the question if there are other issues that may arise. In UniFMU, the representation for these enums is explicitly made i32:

#[repr(i32)]
#[derive(Debug, PartialEq, Clone, Copy, IntoPrimitive, TryFromPrimitive)]
pub enum Fmi3IntervalQualifier {
    IntervalNotYetKnown = 0,
    IntervalUnchanged = 1,
    IntervalChanged = 2,
}

So, might this lead to other issues on platforms like linux/osx? I guess since these are predefined in the standard and there are only a handful of values, it shouldn’t?"

"Should we drop this lib and just manually write the cbindings?

Seems we're encountering issues in second degree dependencies 😄.

This commit is where I deleted a bunch of code for loading and interacting with the FMU that doesn't use the rust-fmi package...: https://github.com/INTO-CPS-Association/unifmu/commit/2fdc44ffd60dca4e599bb03fc31663d999587187

Do you think this is wise Yon?"

clagms commented 8 hours ago

Ah thank you! Then we've decided to drop rust_fmi, so we'll close this issue once we've migrated to fmpy @sagilar

sagilar commented 8 hours ago

Alright. I will keep for now the changes which make the tests compile.