Added I2CSlave support for lpc812

[Willem23]

The I2C engine on newer devices such as the lpc812, lpc824 and lpc1549 is very different from other mbed NXP devices such as the lpc1768. The newer devices have separate Master and Slave engines. This also means that dedicated I2C Slave byte read and byte write functions need to be called from the 'common' mbed I2CSlave.cpp.

[0xc0170]

This proves that there should be slave read/write. I suggest creating new functions in HAL, as you did, but for all targets. So the i2c slave class invokes them and there's not specific target keywords there.

You can edit this PR - leave implementation for lpc812 slave (file libraries/mbed/targets/hal/TARGET_NXP/TARGET_LPC81X/i2c_api.c, the other 2 files - revert changes), send a new pull request with modifications to introduce slave read/write, and enable slave for this target.

What do u think?

[Willem23]

Hi Martin,

I agree that we should have the new slave functions for all targets and get
rid of target specific keywords in i2c slave class. That was also my first
approach. However, this means all targets must be updated to implement:

int i2c_slave_byte_read(i2c_t *obj, int last);

int i2c_slave_byte_write(i2c_t *obj, int data);

It should not be too difficult to do as it can be a wrapper around the
current master byte read/write. However, I don't think I have the cycles to
do that myself in the near term and more importantly I don't have the target
hardware to verify that it works correctly. So I decided to go for target
keywords as quick workaround. This way we don't break the existing code, I
can add the lpc812 now and the other platforms can be adapted incrementally
until all have transitioned. Once that has happened the target keywords can
be removed again. I know, may never happen.

I also think that transition should best be done after some
reviewing/rethinking of the current I2C API and perhaps add or change some
more functions eg the start_address() discussed before and add an additional
pointer parameter to return status in the i2c_read_byte() function. These
mods could then all be implemented at the same time.

However, perhaps I misunderstand your proposal, so I am OK with any better
solution.

Regards,

Wim

---

From: Martin Kojtal [mailto:[email protected]]
Sent: woensdag 4 februari 2015 9:23
To: mbedmicro/mbed
Cc: Wim
Subject: Re: [mbed] Added I2CSlave support for lpc812 (#881)

This proves that there should be slave read/write. I suggest creating new
functions in HAL, as you did, but for all targets. So the i2c slave class
invokes them and there's not specific target keywords there.

You can edit this PR - leave implementation for lpc812 slave (file
libraries/mbed/targets/hal/TARGET_NXP/TARGET_LPC81X/i2c_api.c, the other 2
files - revert changes), send a new pull request with modifications to
introduce slave read/write, and enable slave for this target.

What do u think?

Reply to this email directly or view it
#881 (comment) on
GitHub.
<https://github.com/notifications/beacon/AKMUNFUOGFaMcplNCAE4xzZzKf4-1tfKks5
noc5YgaJpZM4DbNGC.gif>

[0xc0170]

My proposal is to keep changes in HAL, the slave implementation, device i2c slave macro set to 0 to disable it, remove macros from I2C slave class.
I might find time to split master/slave read/write for each target. Then we can enable i2c slave for your target.

[Willem23]

Martin,

I have removed the lpc812 specific macro in the I2C slave class again, I
just left some comments in there as reminder that there is a problem that
needs fixing.

I have currently disabled the i2c slave in device.h for the lpc812, but in
fact you can enable it and it works fine when you use the block read/writes.
The problem occurs only when you use the byte read/writes for the slave
mode. These methods need the fixes in the I2C slave class that I have
proposed and prepared for the lpc812 by implementing i2c_slave_byte_read and
i2c_slave_byte_write. Please proceed and merge the pull.

Regards,

Wim

---

From: Martin Kojtal [mailto:[email protected]]
Sent: vrijdag 6 februari 2015 9:43
To: mbedmicro/mbed
Cc: Wim
Subject: Re: [mbed] Added I2CSlave support for lpc812 (#881)

My proposal is to keep changes in HAL, the slave implementation, device i2c
slave macro set to 0 to disable it, remove macros from I2C slave class.
I might find time to split master/slave read/write for each target. Then we
can enable i2c slave for your target.

Reply to this email directly or view it
#881 (comment) on
GitHub.
<https://github.com/notifications/beacon/AKMUNHhFHzryWTttKdljknVkPo1Ljldiks5
npHYrgaJpZM4DbNGC.gif>

[0xc0170]

Ok great, one more thing, to remove those comments from libraries/mbed/common/I2CSlave.cpp file. The code should stay clean, the place for this type of reminders is our issue tracker

I created a new issue with the code snippet #896

[Willem23]

Removed comments in I2CSlave.

[0xc0170]

Thanks for cooperating, going to merge it