Implement I2C using generics instead of trait objects

[JADarius]

Pull Request Overview

This pull request fixes #6. It is still a work in progress, but I need help with the implementation.

TODO or Help Wanted

This pull request still needs code review for what I've already done and I want help with the implementation strategy.

• [x] use generics for I2C
• [x] use generics for SmBus
• [x] implement a NoSMBus type
• [x] make it compile for all the boards
• [x] observe de binary sizes for imix, microbit and risc-v board

Documentation Updated

• [ ] No updates are required.

Formatting

• [x] Ran make prepush.

[alexandruradovici]

It works, we have to change &dyn I2CDevice to use a trait bound.

[JADarius]

I'm sorry, but I don't understand what I have to do.

[alexandruradovici]

You must modify all the usages of &dyn I2CDevice to to use a trait bound I: I2CDevice.

[JADarius]

Where should I look for &dyn I2CDevice? I can't find the files that have that &dyn I2CDevice.

[alexandruradovici]

Just some examples:

https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/6f91e8df807976582513ff62e571622ef4df66b6/capsules/src/lsm303agr.rs#L145-L146

https://github.com/search?q=repo%3AUPB-CS-OpenSourceUpstream%2Ftock%20%26%27a%20dyn%20i2c%3A%3AI2CDevice&type=code

[JADarius]

I changed all the &dyn I2CDevice that I've found, but I get a compilation error in all of the components affected by the change: expected type parameter 'J', found struct 'capsules::virtual_i2c::I2CDevice and I don't understand where I made the mistake.

[alexandruradovici]

The problem is the following: you have two types:

• a trait called I2CDevice defined in https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/1be6f52a6da75276dff24b61b30ae70a56c6f198/kernel/src/hil/i2c.rs#L217
• a structure that implements a virtual I2CDevice defined in https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/1be6f52a6da75276dff24b61b30ae70a56c6f198/capsules/src/virtual_i2c.rs#L211

You need to be careful which one is used. The adps9960 driver uses the I2CDevice structure, which in terms uses the I2CDevice trait. You do not need the extra generic here, because you know the actual data type that you will use (the I2CDevice structure). The only template that you need is the I2CMaster, which you have correctly used.

https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/1be6f52a6da75276dff24b61b30ae70a56c6f198/capsules/src/virtual_i2c.rs#L254

When it comes it the actual sensor, your modifications are correct, it receives any structure that implements the I2CDevice trait. The component will use a I2CDevice<'static, I> structure, which does implement the I2CDevice trait.

[JADarius]

I have a question: how do I implement the static components macros? Right now they give me errors when I try to compile for the microbit.

[alexandruradovici]

You have to add a parameter for the I2C type to every component that uses the I2C. Take a look at components that use SPI. The difference is that SPI has two generic types.

[JADarius]

The code compiles for the microbit now, but I'm not sure that I have made the correct modifications, especially in the microbit's main.rs. How do I test if the I2C works?

[alexandruradovici]

The easiest way to test is to run the sensors application. If it displays the motion sensor, it means it should work fine.

[JADarius]

I've run the sensors application and it gave me the following output: Temperature: 25 deg C Acceleration: X: -19 Y: -750 Z: 683 Magnetometer: X: 3742 Y: -1516 Z: 389 Sound Pressure: 67 I've also run the sensors application on an unmodified version of Tock and it looks the same.

[JADarius]

I have also checked the sizes of microbit_v2 from both of the versions. The unmodified one has 4225184 bytes and the one that uses generics has 4203324 bytes.

[alexandruradovici]

This seems to work. We still need to fix the SMBus in https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/1be6f52a6da75276dff24b61b30ae70a56c6f198/capsules/src/virtual_i2c.rs#L17.

This is a different type of I2C. The problem is that it is in an Option, so it is not always used. This means that for boards that do not have SMBus, like the microbit v2, there is no way to supply an actual type.

My suggestion is to define a NoSMBus type like:

struct NoSMBus;

impl SMBusMaster for NoSMBus {
  // all functions return Err(ErrorCode::NOSUPPORT)
}

in https://github.com/UPB-CS-OpenSourceUpstream/tock/blob/master/kernel/src/hil/i2c.rs.

Use this type as the default type for the smbus like:

pub struct MuxI2C<'a, I: i2c::I2CMaster, S: i2c::SMBusMaster = NoSMBus> {
    i2c: &'a I,
    smbus: Option<&'a S>,
   // ...
}

[JADarius]

I modified the code and I think that it compiles for all the boards (make prepush says that all checks passed). I had one problem: the nRF52840 board needed a Master Slave interface and TWI didn't explicitly implement the hil::i2c::I2CMasterSlave trait so I did a hack where I implement it in the implementation of I2C for nRF52 and I don't know if it was a good idea.

[alexandruradovici]

Looks good. Can you check the code size (printed by make) for a few boards?

[JADarius]

Old imix: text data bss dec hex 175092 0 28568 203660 31b8c New imix: text data bss dec hex 174836 0 28556 203392 31a80

Old microbit: text data bss dec hex 106500 0 15788 122288 1ddb0 New microbit: text data bss dec hex 106500 0 15772 122272 1dda0

Old stm32f3discovery: text data bss dec hex 100356 0 16632 116988 1c8fc New stm32f3discovery: text data bss dec hex 100356 0 16616 116972 1c8ec

[alexandruradovici]

Can you check the code sizes for a risc-v board?

[JADarius]

I chose a hifive board to test the sizes. Unmodified version: text data bss dec hex 77388 12 7320 84720 14af0 Modified version: text data bss dec hex 75276 12 6932 82220 1412c

[alexandruradovici]

I suggest rebasing this, fixing the errors and sending it to upstream. Please write a PR message explaining all the modifications.

[alexandruradovici]

@JADarius any progress here?

[alexandruradovici]

@JADarius ping?

[JADarius]

I rebased it and solved the problems with my code but make prepush gives me an error about a function I didn't touch. The error is in the file kernel/src/process_standard.rs on line 246.

[JADarius]

I have made a PR: https://github.com/tock/tock/pull/3431