Discussion:
Interfacing sensors behind I2C mux
(too old to reply)
giorgio.gross@robodev.eu [nuttx]
2018-01-09 15:25:47 UTC
Permalink
Hi,


I aim to interface two identical proximity sensors via I2C (with identical I2C addresses) which are both connected to an I2C multiplexer (PCA9549BDP by NXP). I wrote drivers for both the multiplexer and the sensors, but I'm wondering how to select a sensor through the mux in a clean way:
Currently, the sensors are registered under "/dev/vcnl0"; reading one sensor requires opening the device, selecting the right port of the mux and then performing the read call. Reading the second sensor requires just to select the other mux port and then performing again a read call. Yet, the same file descriptor is used as before - it is not visible that the actual device read from is a different one (except that another mux port was selected).


I don't think this is a clean solution, it would be more convenient to have e.g. two sensors registered "/dev/vcnl0" and "/dev/vcnl1" and open/read from each one separately. But from my point of view that requires polluting the proximity sensor driver with code depending on the mux..


Another solution I can think of is to divide the sensor driver in upper and lower half. The upper half would then contain the mux port selection and the lower half would still be architecture independent. But I'm not sure if this is what the lower/upper half idea is intended for.


Is there a built in pattern / best practice to solve such problems? Please point me in the right direction if I have overlooked a post targeting that problem.


Thanks, Giorgio
Alan Carvalho de Assis acassis@gmail.com [nuttx]
2018-01-09 16:46:27 UTC
Permalink
Hi Giorgio,

You inverted the upper/lower logic. The upper half is the hardware
independent code and the lower half is the hard dependent code. Please
look at some examples of drivers that uses it, like ADC, CAN, etc

I think you could to implement the mux driver in a way it could be
usable by any other I2C driver. Maybe Greg could give you a better
solution, but an option is your mux driver re-use the i2c sensor
driver. You register your sensor driver and then you passes it to mux
driver, that will register a new /dev/muxsensorX and will de-attach
and hidden the original /dev/sensorX.

This way your original sensor driver still independent of the mux (can
work without the mux) and you can use it with the mux.

BR,

Alan
Post by ***@robodev.eu [nuttx]
Hi,
I aim to interface two identical proximity sensors via I2C (with identical
I2C addresses) which are both connected to an I2C multiplexer (PCA9549BDP by
NXP). I wrote drivers for both the multiplexer and the sensors, but I'm
Currently, the sensors are registered under "/dev/vcnl0"; reading one
sensor requires opening the device, selecting the right port of the mux and
then performing the read call. Reading the second sensor requires just to
select the other mux port and then performing again a read call. Yet, the
same file descriptor is used as before - it is not visible that the actual
device read from is a different one (except that another mux port was
selected).
I don't think this is a clean solution, it would be more convenient to have
e.g. two sensors registered "/dev/vcnl0" and "/dev/vcnl1" and open/read from
each one separately. But from my point of view that requires polluting the
proximity sensor driver with code depending on the mux..
Another solution I can think of is to divide the sensor driver in upper and
lower half. The upper half would then contain the mux port selection and the
lower half would still be architecture independent. But I'm not sure if this
is what the lower/upper half idea is intended for.
Is there a built in pattern / best practice to solve such problems? Please
point me in the right direction if I have overlooked a post targeting that
problem.
Thanks, Giorgio
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-01-09 16:59:34 UTC
Permalink
hello


I think that an even better solution is to create a driver for the i2cmux that
gives you multiple virtual i2cbuses.

The i2c operations associated with these busses will select the correct bus
before sending the i2c transactions.

After this you can just register any device (sensor or not) on the virtual bus,
and the transaction() functions will do the magic.


Imagine this is how you register a sensor on a normal i2c bus:


struct i2c_master_s * main_i2c_bus = platform_get_i2c_bus(bus_index);

register_my_i2c_sensor(main_i2c_bus, address, minor);


Then, using a bus mux you would do something like this code:


struct i2c_master_s * main_i2c_bus = platform_get_i2c_bus(bus_index);

struct i2c_master_s * virtual_bus;

struct i2c_mux_s mux;

register_i2c_mux(&mux, main_i2c_bus);


virtual_bus = i2c_mux_get_bus(&mux, 0);

register_my_i2c_sensor(virtual_bus, address, minor1);


virtual_bus = i2c_mux_get_bus(&mux, 1);

register_my_i2c_sensor(virtual_bus, address, minor2);


Best regards

Sebastien
Post by Alan Carvalho de Assis ***@gmail.com [nuttx]
 
Hi Giorgio,
You inverted the upper/lower logic. The upper half is the hardware
independent code and the lower half is the hard dependent code. Please
look at some examples of drivers that uses it, like ADC, CAN, etc
I think you could to implement the mux driver in a way it could be
usable by any other I2C driver. Maybe Greg could give you a better
solution, but an option is your mux driver re-use the i2c sensor
driver. You register your sensor driver and then you passes it to mux
driver, that will register a new /dev/muxsensorX and will de-attach
and hidden the original /dev/sensorX.
This way your original sensor driver still independent of the mux (can
work without the mux) and you can use it with the mux.
BR,
Alan
Post by ***@robodev.eu [nuttx]
Hi,
I aim to interface two identical proximity sensors via I2C (with identical
I2C addresses) which are both connected to an I2C multiplexer (PCA9549BDP by
NXP). I wrote drivers for both the multiplexer and the sensors, but I'm
Currently, the sensors are registered under "/dev/vcnl0"; reading one
sensor requires opening the device, selecting the right port of the mux and
then performing the read call. Reading the second sensor requires just to
select the other mux port and then performing again a read call. Yet, the
same file descriptor is used as before - it is not visible that the actual
device read from is a different one (except that another mux port was
selected).
I don't think this is a clean solution, it would be more convenient to have
e.g. two sensors registered "/dev/vcnl0" and "/dev/vcnl1" and open/read from
each one separately. But from my point of view that requires polluting the
proximity sensor driver with code depending on the mux..
Another solution I can think of is to divide the sensor driver in upper and
lower half. The upper half would then contain the mux port selection and the
lower half would still be architecture independent. But I'm not sure if this
is what the lower/upper half idea is intended for.
Is there a built in pattern / best practice to solve such problems? Please
point me in the right direction if I have overlooked a post targeting that
problem.
Thanks, Giorgio
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-01-09 17:14:15 UTC
Permalink
PS because of yahoo delays, my message crossed with the ones from Greg... We
think along the same lines.

Sebastien
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
 
hello
I think that an even better solution is to create a driver for the i2cmux that
gives you multiple virtual i2cbuses.
The i2c operations associated with these busses will select the correct bus
before sending the i2c transactions.
After this you can just register any device (sensor or not) on the virtual bus,
and the transaction() functions will do the magic.
struct i2c_master_s * main_i2c_bus = platform_get_i2c_bus(bus_index);
register_my_i2c_sensor(main_i2c_bus, address, minor);
struct i2c_master_s * main_i2c_bus = platform_get_i2c_bus(bus_index);
struct i2c_master_s * virtual_bus;
struct i2c_mux_s mux;
register_i2c_mux(&mux, main_i2c_bus);
virtual_bus = i2c_mux_get_bus(&mux, 0);
register_my_i2c_sensor(virtual_bus, address, minor1);
virtual_bus = i2c_mux_get_bus(&mux, 1);
register_my_i2c_sensor(virtual_bus, address, minor2);
Best regards
Sebastien
Post by Alan Carvalho de Assis ***@gmail.com [nuttx]
 
Hi Giorgio,
You inverted the upper/lower logic. The upper half is the hardware
independent code and the lower half is the hard dependent code. Please
look at some examples of drivers that uses it, like ADC, CAN, etc
I think you could to implement the mux driver in a way it could be
usable by any other I2C driver. Maybe Greg could give you a better
solution, but an option is your mux driver re-use the i2c sensor
driver. You register your sensor driver and then you passes it to mux
driver, that will register a new /dev/muxsensorX and will de-attach
and hidden the original /dev/sensorX.
This way your original sensor driver still independent of the mux (can
work without the mux) and you can use it with the mux.
BR,
Alan
Post by ***@robodev.eu [nuttx]
Hi,
I aim to interface two identical proximity sensors via I2C (with identical
I2C addresses) which are both connected to an I2C multiplexer (PCA9549BDP by
NXP). I wrote drivers for both the multiplexer and the sensors, but I'm
Currently, the sensors are registered under "/dev/vcnl0"; reading one
sensor requires opening the device, selecting the right port of the mux and
then performing the read call. Reading the second sensor requires just to
select the other mux port and then performing again a read call. Yet, the
same file descriptor is used as before - it is not visible that the actual
device read from is a different one (except that another mux port was
selected).
I don't think this is a clean solution, it would be more convenient to have
e.g. two sensors registered "/dev/vcnl0" and "/dev/vcnl1" and open/read from
each one separately. But from my point of view that requires polluting the
proximity sensor driver with code depending on the mux..
Another solution I can think of is to divide the sensor driver in upper and
lower half. The upper half would then contain the mux port selection and the
lower half would still be architecture independent. But I'm not sure if this
is what the lower/upper half idea is intended for.
Is there a built in pattern / best practice to solve such problems? Please
point me in the right direction if I have overlooked a post targeting that
problem.
Thanks, Giorgio
spudarnia@yahoo.com [nuttx]
2018-01-09 17:25:27 UTC
Permalink
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
I think that an even better solution is to create a driver for the i2cmux that
gives you multiple virtual i2cbuses.


Yes. And again I think that the I/O expander driver and drivers/gpio_lower_half.c are good examples.


drivers/gpio_lower_half.c exposes a "virtual" GPIO pin in the same that some that some I2c_multiplexer_lower_half.c could expose a "I2C bus".


Greg


PS: Sebastien is the author of the I/O expander architecture and a good person to get advice from in this case.
Alan Carvalho de Assis acassis@gmail.com [nuttx]
2018-01-09 19:34:23 UTC
Permalink
Hi Giorgio,

In fact Greg and Sebastien suggestion is even better, this way you
will get a very flexible i2c mux driver.

Just I question, why do you use PCA9549 instead of PCA9547 ?

See: https://hackaday.com/2015/08/12/i2c-bus-splitting-with-a-more-professional-touch/

BR,

Alan
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
I think that an even better solution is to create a driver for the i2cmux that
gives you multiple virtual i2cbuses.
Yes. And again I think that the I/O expander driver and
drivers/gpio_lower_half.c are good examples.
drivers/gpio_lower_half.c exposes a "virtual" GPIO pin in the same that
some that some I2c_multiplexer_lower_half.c could expose a "I2C bus".
Greg
PS: Sebastien is the author of the I/O expander architecture and a good
person to get advice from in this case.
giorgio.gross@robodev.eu [nuttx]
2018-01-11 12:20:10 UTC
Permalink
OK thank you all for your thoughts so far!


To sum up:
I like the idea of providing virtual i2c busses. The mux driver will create i2c_master_s instances and set the i2c_ops_s of each instance. i2c_ops_s functions ( e.g. transfer() ) will then select the right port and then pass the i2c transfer call on to the original i2c_master_s of the mux. This approach would not require to split any driver into upper and lower half; also existing i2c sensor drivers will be able to use the mux just by replacing the original i2c_master_s instance with the virtual i2c_master_instance upon registration.


If I understand correctly, Greg’s approach goes even further and tries to split the sensor driver into upper and lower half, where the upper half provides the application interface and gets a reference to functions of the lower half, just like the gpio.c driver gets a gpio_dev_s with a gpio_operations_s inside. The lower half then is aware of the mux and selects the right port before transferring any data via i2c.
On top of that, the mux is realized through a generic i2cmultiplexer.h interface; that interface provides functions which each mux driver is expected/likely to have ( such as select_port(..) ). That one is implemented by my mux driver; the sensor lower half finally does not even know of my special mux driver but just about the common i2cmultiplexer.h interface (that is passed to it upon registration).
Post by ***@yahoo.com [nuttx]
drivers/gpio_lower_half.c exposes a "virtual" GPIO pin in the same that some that some I2c_multiplexer_lower_half.c could expose a "I2C bus".
Do you mean it instantiates virtual i2c_master_s, registers the i2c sensors and passes that i2c_master_s?




As I see fit, the ioexpander / gpio driver does not fully resemble the sensor driver part of my problem: If I understood correctly, the ioexpander interface was designed to easily add new ioexpander device drivers, but gpio.c will always be the unique gpio driver. In my case, it’s likely that we will have multiple sensor drivers (which correspond to gpio.c) making use of the mux interface. So, integrating a new driver requires to add a new upper half sensor driver and extend or reimplement the lower half sensor driver. This means that none of the existing drivers will be compatible with the mux unless modified prior use with the mux. Also think about having multiple sensors connected to multiple multiplexer (e.g. 2 sensors on mux0 and 2 sensors on mux1)..


To apply the same structure on my problem it would make sense to me to just implement the i2cmux_lower_half.c which registers a new i2c_master_s through i2c_driver.c.
So basically
gpio.c = i2c_driver.c -> will always be the only i2c driver, provides application interface (no changes needed)
gpio.h = i2c_master.h -> used by i2c_driver.c which calls the i2c_master_s.ops.transfer (no changes needed)
ioexpander.h = i2xmux.h -> common interface to mux drivers. Provides transfer_on_port(n, msgs, count) function etc. in i2xmux_dev_s
some_ioexpander_driver.c = my_mux_driver.c -> Gets the original i2c_master_s, instantiates the mux device and returns an i2cmux_dev_s. i2cmux_dev_s.transfer_on_port(n, msgs, count) points to the function implemented by my_mux_driver.c. The implementation selects the port on the mux and then calls original_i2c_master.transfer(original_i2c_master, msgs, count) to resume the i2c transfer.
gpio_lower_half.c = i2cmux_lower_half.c -> gets a i2xmux_dev_s, mux_port number for the virtual i2c bus and a minor. Instantiates a new i2c_master_s with i2c_master_s.ops.transfer pointing to a function implemented by i2cmux_lower_half.c. That function calls the i2cmux_dev_s.transfer_on_port(n, msgs, count). Registers a new i2c bus during initialization through i2c_driver.c.i2c_register as /dev/i2c<minor> (just like the gpio_lower_half.c registers a /dev/gpioN device).
my_i2c_sensor.c will then get the i2c bus returned by platform_get_i2c_bus(bus_index) where this time bus_index is the same as the minor passed to i2cmux_lower_half.c


I think this is a good solution as all sensor drivers can remain the way they are (no modification to upper lower half needed) and the mux is abstracted by i2xmux_dev_s. Basically combining the two solutions by you, Greg and Sebastien. But whatever the final solution will be, I will be happy to implement it! What do you think?




Alan: Yes, I somehow got confused with that lower upper half idea when I wrote it down, but things are clear now. As for the PCA9549 : I don't know why we use exactly that one, it was selected by the engineer responsible for the circuit design. I will ask him the next time I meet him.


Best regards
Giorgio
giorgio.gross@robodev.eu [nuttx]
2018-02-05 15:13:00 UTC
Permalink
I have now implemented the suggested i2c multiplexer abstraction as described in the last message(s); if you want to have a look:

Source files: https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/drivers/i2c/?at=feature-pca9540bdp-i2cmultiplexer https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/drivers/i2c/?at=feature-pca9540bdp-i2cmultiplexer
Headers: https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/include/nuttx/i2c/?at=feature-pca9540bdp-i2cmultiplexer https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/include/nuttx/i2c/?at=feature-pca9540bdp-i2cmultiplexer


I made the following changes to my last suggested solution:
I put i2cmux_lower_half(..) in i2c_master.h (in accordance with gpio.h/gpio_lower_half() ), if this is not okay please let me know; I can also move it to a separate file. Virtual i2c busses are not automatically registered as /dev/i2cN. Instead, an i2c_master_s pointer passed to i2cmux_lower_half(..) instance is set to the virtual i2c_master_s instance; registering the virtual bus is left to the initialization processE.g. our initialization now looks something like this: https://bitbucket.org/snippets/ordsen/BeoGRa https://bitbucket.org/snippets/ordsen/BeoGRa
And this is the i2c sensor driver used with the mux: https://bitbucket.org/ordsen/nuttx/src/8f2e6918cd55f47a804129794dea0eed4dfd2bc1/drivers/sensors/vcnl4040.c?at=feature-vcnl4040-driver&fileviewer=file-view-default https://bitbucket.org/ordsen/nuttx/src/8f2e6918cd55f47a804129794dea0eed4dfd2bc1/drivers/sensors/vcnl4040.c?at=feature-vcnl4040-driver&fileviewer=file-view-default (usage example in apps folder)
But you should be able to used it with any other existing i2c sensor driver without changing the sensor driver.


I will create a PR for the mux interface, the pca9540bdp mux and the vcnl4040 sensor in the days to come.
Your thoughts on my code are appreciated!


Best regards
Giorgio
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-05 15:39:07 UTC
Permalink
hello

(first of all, what follows is just my opinion, I think it contains useful
advices and I hope it is heard in the general interest of the NuttX project, but
all decisions regarding the NuttX architecture belong to Greg).


I am not sure why you need a "i2cmux lower half". I would have expected this to
be integrated directly in the mux device driver. This looks like a very thin
layer added on top of the mux device.

Since this mux device will be instantiated in the board config, you can just add a :

int pca9540bdp_lower_half(FAR struct i2cmultiplexer_dev_s* i2cmux, FAR struct
i2c_master_s** vi2c, uint8_t port)

to your pca driver. I think that it's okay to do that, and that it saves you a
useless lower half driver.


Letting the user register his own /dev entries for the virtual bus is a good
thing, but I cannot understand why you need a dev entry for the mux itself...
this entry is far from necessary, I would have made it optional and I hope Greg
will require that.

Pursuing my MTD similarity: When you create MTD partitions, you need to invoke a
function from the "mtd partition driver" that creates a new MTD partition from a
parent MTD device, then you can register these partitions with the FTL layer or
something else. But the "MTD partition driver" that makes the partitions never
needs a /dev entry by itself.

Thats why the open/close/write routines of this character driver are empty. The
config can be made available to other parts of the system as an ioctl operation
of the i2c_master_s instances. I would prefer adding an optional ioctl interface
to i2c_master_s operations than use this design as-is. It is much more clean
imho. I can help with reviewing such patches, that would affect other i2c master
drivers (just the initialization of the ioctl entries to null).

Also, what is the goal of  PCA9540BDPIOC_SEL_PORTin this driver ? I would have
thought that port switching was automatic? Is there a need to manually switch
the ports? This will just confuse the devices on the virtual ports.

But that's a very good start. Let's polish this.


Sebastien
Post by ***@robodev.eu [nuttx]
 
I have now implemented the suggested i2c multiplexer abstraction as described
Source
files:  https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/drivers/i2c/?at=feature-pca9540bdp-i2cmultiplexer
Headers: https://bitbucket.org/ordsen/nuttx/src/77557554d22f6316f37c7b7a37c7e862c9b9cb2a/include/nuttx/i2c/?at=feature-pca9540bdp-i2cmultiplexer
* I put i2cmux_lower_half(..) ini2c_master.h (in accordance with
gpio.h/gpio_lower_half() ), if this is not okay please let me know; I can
also move it to a separate file.
* Virtual i2c busses are not automatically registered as /dev/i2cN. Instead,
an i2c_master_s pointer passed to i2cmux_lower_half(..) instance is set to
the virtual i2c_master_s instance; registering the virtual bus is left to
the initialization process
E.g. our initialization now looks something like
this: https://bitbucket.org/snippets/ordsen/BeoGRa 
And this is the i2c sensor driver used with the
mux: https://bitbucket.org/ordsen/nuttx/src/8f2e6918cd55f47a804129794dea0eed4dfd2bc1/drivers/sensors/vcnl4040.c?at=feature-vcnl4040-driver&fileviewer=file-view-default (usage
example in apps folder)
But you should be able to used it with any other existing i2c sensor driver
without changing the sensor driver.
I will create a PR for the mux interface, the pca9540bdp mux and the vcnl4040
sensor in the days to come. 
Your thoughts on my code are appreciated!
Best regards
Giorgio
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-05 15:48:27 UTC
Permalink
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
int pca9540bdp_lower_half(FAR struct i2cmultiplexer_dev_s* i2cmux, FAR struct
i2c_master_s** vi2c, uint8_t port)
I meant something more like:


FAR struct i2c_master_s* pca9540bdp_lower_half(FAR struct pca9540bdp_dev_s *dev,
uint8_t port);


Returns null if error, else a pointer to a virtual bus. Then you do whatever you
want with this instance.


To make this work, you can just make struct pca9540bdp_dev_s * visible to the
board initialization code in your pca9540 header. You dont need to define its
contents here, just the name.

Also the initialization could change a bit. Your current code REQUIRES a char
device:

FAR struct i2cmultiplexer_dev_s* pca9540bdp_initialize(FAR const char *devpath,
FAR struct i2c_master_s *i2c, uint8_t addr)

But this is not necessary if you accept that the mux device is only an
"internal" device that just provides virtual i2c port to anyone else:

int pca9540bdp_initialize(FAR struct pca9540bdp_dev_s *dev, FAR struct
i2c_master_s *parentport, uint8_t addr);

Also declared in the pca9540.h file at include/nuttx/i2c


If you REALLY want a char device for your multiplexer (even if I dont think that
is necessary), you can add a register like function:

int pca9540bdp_register(FAR const char *devpath, FAR struct pca9540_dev_s *pcadev);


With these changes, your code would be perfect!


sebastien
giorgio.gross@robodev.eu [nuttx]
2018-02-05 16:56:11 UTC
Permalink
Hi Sebastien,

thanks for the tipps! PCA9540BDPIOC_SEL_PORTin is indeed redundant, I will remove that one.


I registered the PCA under /dev/... as it provides an ioctl call to disable the device and maybe some application might use this. But I did not understand the thing with the MTD partitions, thanks for pointing that out.
I agree that the pca9540 initialize function should not necessarily require a char device if I handle it, as you suggest, as an internal device.


That idea with the ioctl sounds reasonable, might there be any other use cases for ioctl in i2c master instances than just for mux devices?


I sticked to the i2cmux_lower_half because I tried to stay close to the ioexpander (gpio_lower_half...) interface. But yes, it just passes on the calls to the mux device without doing anything.


I will apply some changes and then come back to you again. It might take me a while though as I'm currently preparing for my examinations..


Best regards
Giorgio
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-05 17:06:38 UTC
Permalink
hello

Good luck with your examinations, take your time!

I guess the gpio comparison was just to guide you :) We can actually do
something more suitable here.

At the moment i2c_master_s does not have an ioctl function so that may be a
pretty big change impacting all of NuttX i2c drivers, and possibly useless (Greg
will tell). You can still keep an internal function to disable the switch, and
the optional char driver / user code can wrap that function.

Sebastien
Post by ***@robodev.eu [nuttx]
 
Hi Sebastien,
thanks for the tipps! PCA9540BDPIOC_SEL_PORTin is indeed redundant, I will
remove that one.
I registered the PCA under /dev/... as it provides an ioctl call to disable
the device and maybe some application might use this. But I did not understand
the thing with the MTD partitions, thanks for pointing that out.
I agree that the pca9540 initialize function should not necessarily require a
char device if I handle it, as you suggest, as an internal device. 
That idea with the ioctl sounds reasonable, might there be any other use cases
for ioctl in i2c master instances than just for mux devices?
I sticked to the i2cmux_lower_half because I tried to stay close to the
ioexpander (gpio_lower_half...) interface. But yes, it just passes on the
calls to the mux device without doing anything. 
I will apply some changes and then come back to you again. It might take me a
while though as I'm currently preparing for my examinations..
Best regards
Giorgio
 
giorgio.gross@robodev.eu [nuttx]
2018-02-26 15:41:25 UTC
Permalink
hi, I was able to find some time to add the changes you suggested: https://bitbucket.org/ordsen/nuttx/src/10d7eedb7b0d/drivers/i2c/?at=feature-pca9540bdp-i2cmultiplexer https://bitbucket.org/ordsen/nuttx/src/10d7eedb7b0d/drivers/i2c/?at=feature-pca9540bdp-i2cmultiplexer
https://bitbucket.org/ordsen/nuttx/src/10d7eedb7b0d8ae83045616d688003ba10717c7f/include/nuttx/i2c/?at=feature-pca9540bdp-i2cmultiplexer https://bitbucket.org/ordsen/nuttx/src/10d7eedb7b0d8ae83045616d688003ba10717c7f/include/nuttx/i2c/?at=feature-pca9540bdp-i2cmultiplexer



Instantiation now looks like this:


struct pca9540bdp_dev_s* pca9645_dev = pca9540bdp_initialize(i2c1_master, CONFIG_PCA9540BDP_BASEADDR);
virtual_i2c1_master_0 = pca9540bdp_lower_half(pca9645_dev, PCA9540BDP_SEL_PORT0);
virtual_i2c1_master_1 = pca9540bdp_lower_half(pca9645_dev, PCA9540BDP_SEL_PORT1);
/* now just pass the virtual i2c masters to i2c sensors */





I decided to remove the application interface for now. ioctl is not really required by this device anyway as it doesn't provide any important configuration options..


Do you think that it is cleaner now?


Best regards
Giorgio

spudarnia@yahoo.com [nuttx]
2018-01-09 16:51:08 UTC
Permalink
Hi, Giorgio
Post by ***@robodev.eu [nuttx]
Currently, the sensors are registered under "/dev/vcnl0"; reading one sensor requires opening the device, selecting the right port of the mux and then performing the read call. Reading the second sensor requires just to select the other mux port and then performing again a read call. Yet, the same file descriptor is used as before - it is not visible that the actual device read from is a different one (except that another mux port was selected).
I don't think this is a clean solution, ..
I agree. I think it should be transparent to the application whether it is accessed via the I2C multiplexer or directly via the I2C bus. It would be better not to expose the multiplexer to the application at all.

For most drivers this is handled by board-level logic, a "bottom half" driver that provides the interface to the "upper half" driver. The I2C multiplexer should be hidden behind that board level interface. The presence of the I2C multiplexer is, after all, a board-level design issue and should be transparent to the sensor driver.

In this case, the "upper half" driver would be the sensor driver; the "lower half driver". That lower half driver would:

1. Configure the I2C multiplexer. It would be the single point in the design that is aware of the existence of the multiplexer. It would provide two instances of the sensor lower half: One configured to work with one sensor and one configured to work with the other sensor.

2. I would then register two instances of the the sensor sensor driver.
Post by ***@robodev.eu [nuttx]
... it would be more convenient to have e.g. two sensors registered "/dev/vcnl0" and "/dev/vcnl1" and open/read from each one separately.
Exactly
Post by ***@robodev.eu [nuttx]
But from my point of view that requires polluting the proximity sensor driver with code depending on the mux..
I would think that the proximity sensor should have no knowledge of the mux.
Post by ***@robodev.eu [nuttx]
Another solution I can think of is to divide the sensor driver in upper and lower half. The upper half would then contain the mux port selection and the lower half would still be architecture independent. But I'm not sure if this is what the lower/upper half idea is intended for.
Yes, that is the correct thing to do. Much better architecture than a mish-mash of sensor driver and I2C multiplexer. They are separate devices and there should be no interdependence between them.
Post by ***@robodev.eu [nuttx]
Is there a built in pattern / best practice to solve such problems? Please point me in the right direction if I have overlooked a post targeting that problem.
The upper/lower half driver architecture is the standard for all drivers.

The architecture should be very much like the GPIO expander of drivers/ioexpander. Note that:

1. There is are I/O expander drivers that have no application interfaces. These are configured by board-level logic to control the discrete I/O.

There are a couple of I/O expander drivers there. All provide the common I/O expander interface defined in the file include/nuttx/ioexpander/ioexpander.h... struct ioexpander_dev_s.

In the same way, you should define a common I2C multiplexor interface in include/nuttx/i2c/i2c_multiplexer.h, perhaps called struct i2cmultiplexer_dev_s.

2. There is a separate GPIO driver at drivers/ioexpander/gpio.c. It knows nothing about I/O expanders but provides a driver that can be used by the application to control the GPIO. This is a "top half" driver.

This GPIO driver is analogous to your sensor driver.

3. Boards that support "normal" discrete I/O provide the "bottom half" interface that is defined in include/ioexpander/gpio.h.

In the same way, you should have a "lower half" interface defined for the sensor at include/nuttx/sensors.

4. Now for the magic.. in drivers/ioexpander there is a file called gpio_lower_half.c. It is also prototyped in include/nuttx/ioexpander/gpio.h. This is a special implementation of the GPIO lower half interface that uses the standard I/O expander interface to access GPIO pins. Notice that it takes an instance of the common I/O expander as an input, struct ioexpander_dev_s.

In the same way, you should have a generic I2C multiplexer lower half that takes an instance of truct i2cmultiplexer_dev_s as an input. That then does the job you want in a completely modular manner.

NuttX is all about modularity. The two highest values that I hold for the RTOS are (1) strict compliance to standards as defined at OpenGroup.org, and (2) high modularity. No mish-mash code please.

I can help with this architecture if you like. I can create a break and review and help out as needed so that you can be happy with the result and I can be happy about including a good design into the code base.

Greg
spudarnia@yahoo.com [nuttx]
2018-01-09 16:54:26 UTC
Permalink
Unfortunately, there are no good examples of the use of the I/O expander in the code base that you could use to model a good I2C multiplexer solution.

There is only one and that is for a simulated platform that has no real discrete I/O. That is the file configs/sim/src/sim_ioexpander.c. But you can see the be basics there. In the function sim_gpio_initialize() you can see it configure and register the GPIO lower driver with a I/O multiplexer instance.
Loading...