I2C HAL ideas

[RREE]

I think the hal-i2c.ads lacks some comments on how to use it. Here are some ideas for modifications:

package HAL.I2C is

   type I2C_Status is
     (Ok,
      Error,         -- Err_Error,
      Timeout,       -- Err_Timeout,
      No_Connection, -- ?
      Busy);         -- ?

   subtype I2C_Data is UInt8_Array;

   -- when requesting data from the device, do we (the master) send
   -- one (Memory_Size_8b) or two bytes to the device.
   -- type I2C_Memory_Address_Size is
   --   (Memory_Size_8b,    -- single command or address
   --    Memory_Size_16b);  -- two bytes, BE or LE?


   -- subtype I2C_Address is UInt10;
   -- 7 and 10-bit addressess must be distinct types
   subtype I2C_7bit_Address is UInt7 range 8 .. 16#77#;
   subtype I2C_10bit_Address is Uint10;
   -- do we really support 10bit addresses? If yes, we have to
   -- overload all routines with 7 and 10 bit or add a parameter that
   -- tells us, if we have to handle the given value as a 7 or 10 bit
   -- address.  Honestly I had never seen a real 10bit device. I
   -- propose to only support 7bit addresses

   type I2C_Port is limited interface;

   type Any_I2C_Port is access all I2C_Port'Class;

   -- Create I2C start condition.  Send the Data to the device (slave)
   -- at I2C_Address through the port This.  The number of bytes is
   -- determined by the length of Data.  Ends with I2C stop condition.
   -- Success or failure is reported in Status.
   procedure Master_Transmit
     (This    : in out I2C_Port;
      Addr    : I2C_7bit_Address;
      Data    : I2C_Data;
      Status  : out I2C_Status;
      Timeout : Natural := 1000) is abstract;
   procedure Master_Transmit
     (This    : in out I2C_Port;
      Addr    : I2C_7bit_Address;
      Data    : UInt8; -- avoid tedious wrapping in (1 => ...)
      Status  : out I2C_Status;
      Timeout : Natural := 1000) is abstract;

   --
   --  Is Master_Receive really useful? I cannot imagine a situation
   --  or a device where you can receive data without prior
   --  information to the device.
   --

   -- Create I2C start condition.  Receive Data from the device
   -- (slave) at I2C_Address through the port This.  The number of
   -- expected bytes is determined by the length of Data.  Ends with
   -- I2C stop condition.  Success or failure is reported in Status.
   procedure Master_Receive
     (This    : in out I2C_Port;
      Addr    : I2C_7bit_Address;
      Data    : out I2C_Data;
      Status  : out I2C_Status;
      Timeout : Natural := 1000) is abstract;

   -- Create I2C start condition.  Send the Send_Data to the device
   -- (slave) at I2C_Address through the port This.  The number of
   -- bytes is determined by the length of Send_Data.  Without
   -- intermediate stop request Recv_Data'Length bytes from the same
   -- client.  Ends with I2C stop condition.  Success or failure is
   -- reported in Status.
   procedure Master_Transmit_And_Receive
     (This      : in out I2C_Port;
      Addr      : I2C_7bit_Address;
      Send_Data : I2C_Data;
      Recv_Data : out I2_Data;
      Status    : out I2C_Status;
      Timeout   : Natural := 1000) is abstract;
   procedure Master_Transmit_And_Receive
     (This      : in out I2C_Port;
      Addr      : I2C_7bit_Address;
      Send_Data : UInt8;
      Recv_Data : out I2_Data;
      Status    : out I2C_Status;
      Timeout   : Natural := 1000) is abstract;

   --  See if there is a device at the address Addr. Report presence
   --  (Status = Ok) or absence (Status = No_Connection). Detects the ACK after sending the address.
   procedure Detect_Connection
     (This      : in out I2C_Port;
      Addr      : I2C_7bit_Address;
      Status    : out I2C_Status) is abstract;


   --
   --  In my opinion we don't need these routines. We can keep them
   --  for compatibility with the existing interface
   --

   --  -- Same as Master_Transmit.  Depending on Mem_Addr_Size one or two
   --  -- bytes from Mem_Addr are prepened before sending Data. Ends with
   --  -- I2C stop condition.
   --  procedure Mem_Write
   --    (This          : in out I2C_Port;
   --     Addr          : I2C_7bit_Address;
   --     Mem_Addr      : UInt16;
   --     Mem_Addr_Size : I2C_Memory_Address_Size;
   --     Data          : I2C_Data;
   --     Status        : out I2C_Status;
   --     Timeout       : Natural := 1000) is abstract;

   --  -- Depending on Mem_Addr_Size first send one or two bytes from
   --  -- Mem_Addr to the slave at address I2C_Address. Then receive
   --  -- Data'Length bytes in Data. Ends with I2C stop condition.
   --  procedure Mem_Read
   --    (This          : in out I2C_Port;
   --     Addr          : I2C_7bit_Address;
   --     Mem_Addr      : UInt16;
   --     Mem_Addr_Size : I2C_Memory_Address_Size;
   --     Data          : out I2C_Data;
   --     Status        : out I2C_Status;
   --     Timeout       : Natural := 1000) is abstract;

end HAL.I2C;

[RREE]

An alternative design adds a parameter to the Master_* routines to send or not to send the Stop condition at the end (defaulting to true).

[RREE]

If we want to reuse basic routines for sending and receiving for master and for slave implementation we need to have simple send and receive routines and a separate stop routine anyway

[hgrodriguez]

I love the suggestion for procedure Detect_Connection

I would like to see Slave_* functions/procedures.

Having said this, and I only want to be an I2C slave, then I need to implement the Master* just for the sake of it.

Could it be maybe better to have:
HAL.I2C (defines all data types and common things between Master/Slave) [could include the Detect_Connection]
HAL.I2C_Master (or HAL.I2C.Master?): define the Master related stuff
HAL.I2C_Slave (or HAL.I2C.Slave?): define the Slave related stuff

Just throwing ideas around.

[RREE]

I'm in favor of having separate child packages for Slave (HAL.I2C.Slave) and Master (HAL.I2C.Master) and a common parent package (HAL.I2C) with type defs and perhaps the commonly needed low level routines Send_Data and Receive_Data without the address part and a parameter what to do with ACK (send, not send, ignore missing ACK, etc.)

I never programmed a slave and don't know what is needed. We can probably copy the idea from C or Python APIs.

[hgrodriguez]

regarding slave: I found some C code which claims to work, but I just did not have the time yet, but it is on my to do list

[JeremyGrosser]

If we're going to consider breaking HAL compatibility, forcing every implementation and consumer of the HAL drivers to update, I think there are a few other things to keep in mind.

We can leverage Alire's crate versioning to differentiate a 1.x branch of HAL from 0.x, or we could just create a new hal2 crate. Either way, this is going to cause compatibility issues and fragmentation of the Ada embedded ecosystem. If we go down this route, we should make a list of every crate that uses or implements HAL and systematically update them all to the new interface as quickly as possible.

The HAL interfaces currently cannot be verified by gnatprove as they use constructs not allowed in SPARK. I think at least making this an option for HAL implementations is a worthwhile goal.

I'd love to see a test suite similar to ACATS, but for HAL driver implementations. I don't know exactly what this would look like, but I'm imagining a hardware-in-the-loop solution with two microcontrollers connected to each other, one running tests, the other verifying that it's sending the right signals at the right time. It may be possible to do this entirely within qemu, which would be great for automated integration testing.

Specific to the proposed I2C interface changes, I don't think removing support for 10 bit addresses is a good idea. While uncommon, these devices do exist and most I2C controllers support them well.

The Detect_Connection procedure is not standardized and not part of the I2C specification, so I don't think it makes sense to require I2C controller drivers to support it. Especially when this functionality can be implemented trivially by making a Master_Transmit call and checking the Status value.

I do wonder if there is space for a higher level Probe interface that all HAL drivers might implement, similar to Linux kernel modules. I can also imagine wanting some power management primitives along those lines, like Linux's module suspend callbacks.

In summary, I think breaking the HAL interface is a bigger challenge than this proposal makes it seem and we should consider the broader implications of these changes before diving in with diffs and pull requests.

[RREE]

Breaking the the interface should come as a last resort. But the interface as it is has flaws that can easily be mitigated by some documenting comments. I have my proposal now in hal-i2c.ads and hal-i2c-master.ads.

Detect_Connection is not standardized, neither are Mem_Read or Mem_Write. And at least Mem_Write can easily implemented by Master_Transmit. These are convenience routines same as Detect_Connection. BTW the naming of these routines only fits if the device is some memory like an EEPROM. There are sensors as I2C devices that expect in the first byte a command, in the second byte a command argument or modifier and then they answer with the measurement values. Calling Mem_Read works but feels quite awkward.

As for the addresses, the 7bit addresses and 10bit addresses are separate address spaces. There can be devices with the 10bit address 0x40 and with the 7bit address 0x40 on the same bus. The current interface does not permit to distinguish between the two. We either need an additional parameter I2C_Address_Size or have two distinct address types and overload the routines appropriately.

[hgrodriguez]

I do agree, that breaking existing code should be the last option. I would strongly suggest, that the crate versioning should help here. But this requires very strict references in the client code using HAL. I learnt to my surprise, that bumping up a version seems to be not fully followed for all changes, which is not something I would recommend for real life systems. So maybe this is a chance to help the community to adapt strict versioning (which should always be the case to begin with, even removing a space shall be a patch level version very like the Maven world in Java with POM.XML where we use alire.toml as an equivalent)

[Fabien-Chouteau]

If we're going to consider breaking HAL compatibility, forcing every implementation and consumer of the HAL drivers to update, I think there are a few other things to keep in mind.

I think the ecosystem is still small enough to make this kind of changes. And as you said, leveraging on semantic versioning to make this easier for everyone. I am against a hal2 crate by the way, it should be hal 0.4.z.

The HAL interfaces currently cannot be verified by gnatprove as they use constructs not allowed in SPARK. I think at least making this an option for HAL implementations is a worthwhile goal.

I agree that this is a worthwhile goal, I don't know which part of the current HAL.I2C for instance are incompatible with SPARK.

I'd love to see a test suite similar to ACATS, but for HAL driver implementations. I don't know exactly what this would look like, but I'm imagining a hardware-in-the-loop solution with two microcontrollers connected to each other, one running tests, the other verifying that it's sending the right signals at the right time.

That's also a good idea, difficult to put in place in my opinion but you already did a great job testing the rp2040_hal.

It may be possible to do this entirely within qemu, which would be great for automated integration testing.

The problem with QEMU is that adding support for a given MCU is a lot of work.

I think the hardware way and the RP2040 with PIO makes it even more doable.

I see a driver tester board with an Rpi Pico and 2.54mm headers: one pin for ground, two for serial in/out and the rest is configuration (all RP2040 IO).
The MCU under test would send commands like:

• I want to perform an I2C controller (master) test
• I will use pin X for SDL, pin Y for SCL
  The driver tester would then configure itself accordingly, using PIO to be able to use any pins.

Or maybe the other way around, the driver tester sends commands to the MCU under test:

• I will test your I2C controller driver
• Tell me which pin is SDL and which one is SCL.

Then the driver tester send test instructions like:

• Send 3 bytes to device 16#42#

We could provide not only the driver tester but also a MCU agnostic piece of code that execute driver tester commands.
So one only has to fill in the configuration of devices for the MCU under test.

The Detect_Connection procedure is not standardized and not part of the I2C specification, so I don't think it makes sense to require I2C controller drivers to support it. Especially when this functionality can be implemented trivially by making a Master_Transmit call and checking the Status value.

I agree, and the HAL should stay as low level as possible in my opinion. Close to the protocol.
One thing to keep in mind as well is the possibility to introduce transparent DMA transfers like I did in the STM32 I2C: https://github.com/AdaCore/Ada_Drivers_Library/tree/master/arch/ARM/STM32/drivers/i2c_stm32f4

I do wonder if there is space for a higher level Probe interface that all HAL drivers might implement, similar to Linux kernel modules.

Sound like this would rely a lot on dynamic memory, it is probably doable but I see this as a library built on top of hal rather than a part of it.

I can also imagine wanting some power management primitives along those lines, like Linux's module suspend callbacks.

I am afraid it will be impossible to make this MCU agnostic. What we could do however, is to define an optional power management interface.

package HAL.Power is

   type Instance is interface;
   subtype Class is Instance'Class;

   type State_Kind is (Power_Up, Power_Down, Suspended);

   function Support (This : Instance; Kind : State_Kind) return Boolean is abstract;

   function State (This : Instance) return State_Kind is abstract
     with Post'Class => This.Support (State'Result);

   procedure Up (This : in out Instance) is abstract
     with Pre'Class  => This.Support (Power_Up),
          Post'Class => This.State = Power_Up;

   procedure Down (This : in out Instance) is abstract
     with Pre'Class  => This.Support (Power_Down),
          Post'Class => This.State = Power_Down;
   
   procedure Suspend (This : in out Instance) is abstract
     with Pre'Class  => This.Support (Suspended),
          Post'Class => This.State = Suspended;end HAL.Power;

   type I2C_Controller is new HAL.I2C.I2C_Port and HAL.Power.Instance with private;

[hgrodriguez]

@Fabien-Chouteau
I do like the proposal of, this would make such a difference!:
I see a driver tester board with an Rpi Pico and 2.54mm headers: one pin for ground, two for serial in/out and the rest is configuration (all RP2040 IO).
The MCU under test would send commands like:

I want to perform an I2C controller (master) test
I will use pin X for SDL, pin Y for SCL
The driver tester would then configure itself accordingly, using PIO to be able to use any pins.
Or maybe the other way around, the driver tester sends commands to the MCU under test:

I will test your I2C controller driver
Tell me which pin is SDL and which one is SCL.
Then the driver tester send test instructions like:

Send 3 bytes to device 16#42#
We could provide not only the driver tester but also a MCU agnostic piece of code that execute driver tester commands.
So one only has to fill in the configuration of devices for the MCU under test.

[JeremyGrosser]

The HAL interfaces currently cannot be verified by gnatprove as they use constructs not allowed in SPARK. I think at least making this an option for HAL implementations is a worthwhile goal.

I agree that this is a worthwhile goal, I don't know which part of the current HAL.I2C for instance are incompatible with SPARK.

Using Local_Configuration_Pragmas to add pragma SPARK_Mode (On) to the hal crate and run gnatprove, I get this error for the HAL.I2C spec.

hal-i2c.ads:51:09: error: general access-to-variable type is not allowed in SPARK
   51 |   type Any_I2C_Port is access all I2C_Port'Class;
      |        ^~~~~~~~~~~~

Full output here: https://gist.github.com/JeremyGrosser/b805aab9f3f94533679b5453e0884afa

[Fabien-Chouteau]

Right, you can't even declare the access type ^^

[RREE]

I want to return to the discussion about the I2C addresses. There are two distinct address spaces (7bit and 10bit addresses) and there are two common ways to store 7bit addresses, either in the lower 7 bits 6:0 or in the upper 7 bits 7:1 of a byte. It is the same information, just differently represented in a byte. The latter is sometimes called a 8bit address.
The current interfaces specifies

subtype I2C_Address is UInt10;

I found no comment or documentation if that is intended to be a 7, 8, or 10bit address. I only learned by looking at the code in rp2040_hal that the users of hal-i2c assume that it is a 8bit address. (In the implementation of RP.I2C_Master the incoming address gets divided by 2, i.e. shifted to right before storing it in a register)
I propose to have all three types declared in a future hal-i2c:

   -- 7 and 10-bit addresses are distinct types
   subtype I2C_7bit_Address is UInt7 range 8 .. 16#77#;
   
   -- 8-bit addresses are actually 7-bit addresses shifted by 1 bit
   subtype I2C_8bit_Address is UInt8 range 16#10# .. 16#EE#;

   subtype I2C_10bit_Address is UInt10;

   subtype I2C_Address is I2C_8bit_Address;
   -- for backward compatibility to hal-0.3.0

There a now several design choices:

1. Use the current subtype definition of I2C_Address and add a parameter to all routines if it is a 7, 8, or 10bit address

   type I2C_Address_Kind is (I2C_7bit, I2C_8bit, I2C_10bit);

   procedure Transmit
     (This      : in out I2C_Master_Port;
      Addr      : I2C_Address;
      Data      : I2C_Data;
      Status    : out I2C_Status;
      Addr_Kind : I2C_Address_Kind := I2C_8bit;
      Timeout   : Natural := 1000) is abstract;

2. Overload all Transmit, Receive, M̀em_Write, and Mem_Read with the different address types.

   procedure Transmit
     (This      : in out I2C_Master_Port;
      Addr      : I2C_7bit_Address;
      Data      : I2C_Data;
      Status    : out I2C_Status;
      Timeout   : Natural := 1000) is abstract;
   procedure Transmit
     (This      : in out I2C_Master_Port;
      Addr      : I2C_8bit_Address;
      Data      : I2C_Data;
      Status    : out I2C_Status;
      Timeout   : Natural := 1000) is abstract;
   procedure Transmit
     (This      : in out I2C_Master_Port;
      Addr      : I2C_10bit_Address;
      Data      : I2C_Data;
      Status    : out I2C_Status;
      Timeout   : Natural := 1000) is abstract;

These two alternatives can have tree variants:
a) use all three types (7, 8, 10)
b) get rid of 7bit, use only 8 and 10 bits
c) get rid of 8bit, use only 7 and 10 bits

The advantage of alternative 1) is that it is the least intrusive. I don't like it as it weakens the type system, you have to specify the actual type in a additional parameter. That is not Ada-style. Alternative 2 is more work but we get strong typing.

As for the variants, my favorite is c). That is what most libraries and documents do

[Fabien-Chouteau]

I am worried about exponential number of sub-programs.
I would go for multiple Start procedure with different address argument types.

[RREE]

I don't think that limiting the address choice to a start procedure helps a lot. The very first byte must already encode in bit 0 if the following action is either a read or a write. That alone requires us to have at least the four cases

1. 7bit-addr transmit
2. 10bit-addr transmit
3. 7-bit-addr receive
4. 10bit-addr receive
   Having a parameter to send or not to send the stop sequence these for routines would be enough to build all other routines on top of them.