espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.8k stars 748 forks source link

I2C/SPI slave #565

Closed gfwilliams closed 4 years ago

gfwilliams commented 9 years ago

JS execution speed is really too slow to properly do something via an IRQ, but there could be an array of data that can be read out...

write X, read N bytes -> get Array[X .. X+N-1]
write X,Y,Z -> Array[X]=Y, Array[X+1]=Z

http://forum.espruino.com/conversations/273112/

gfwilliams commented 4 years ago

@MaBecker: I2C Slave support has just been added to nRF52 builds with 77a60d935908c8837465a59cbb905407ec2e47b4

Use as follows:

Add this to the BOARD.py makefile section:

'DEFINES += -DI2C_SLAVE -DTWIS_ENABLED=1 -DTWIS1_ENABLED=1' # enable I2C slave support

Then on the device:

I2C1.setup({scl:D22,sda:D23,addr:20});
I2C1.on('read',x=>print("read",x));
I2C1.on('write',x=>print("write",x));

print(I2C1.buffer)

So there's a 64 byte Uint8Array called buffer added to I2C1 when addr:xyz is supplied. Then you get events when a read or write has happened, and you can read/write what is in the array (which will have already changed after the write has happened).

On the I2C master it's like every other I2C device:

var i = new I2C();
i.setup({scl:D14,sda:D15});
function writeData(addr, data) {
  i.writeTo(20,addr,data);
}
function readData(addr, len) {
  i.writeTo(20,addr);
  return i.readFrom(20,len);
}
MaBecker commented 4 years ago

This what has been tested with a Pixel badge 2018 and a BBB as master.

I2C1.setup({scl:D10,sda:D11,addr:20});
I2C1.buffer[0]=255;
I2C1.on('read',x=>print("read",x));
I2C1.on('write',x=>print("write",x));
//I2C1.on('write',()=>g.drawString(I2C1.buffer[0].toString(),0,0,true).flip());

print(I2C1.buffer);

Overall: Amazing work - thanks a lot.

MaBecker commented 4 years ago

I2CS is causing timeouts

workaround: reboot the badge

MaBecker commented 4 years ago

I2CS permantent reads

gfwilliams commented 4 years ago

Try now - the lack of response and and re-initialisation should be fixed

MaBecker commented 4 years ago

Great - that fixed those two issues, as you expected - thanks

MaBecker commented 4 years ago

Any idea how to nail this down?

watch -n 1 i2cdetect -y -a -r 1, reads up to addr 63 and than cause timeouts by the client on the i2c bus

MaBecker commented 4 years ago

Just digging into it with some jsiConsolePrintf()

in work......

    case TWIS_EVT_READ_REQ:
        if (p_event->data.buf_req) {
          JsVar *i2c = jsvObjectGetChild(execInfo.root,"I2C1",0);
          if (i2c) {
            JsVar *buf = jsvObjectGetChild(i2c,"buffer",0);
            size_t bufLen;
            char *bufPtr = jsvGetDataPointer(buf, &bufLen);
            jsiConsolePrintf("Case TWIS_EVT_READ_REQ %d, %d\n",twisAddr, bufLen - twisAddr);
            if (bufPtr && bufLen>twisAddr)
              nrf_drv_twis_tx_prepare(&TWIS1, bufPtr + twisAddr, bufLen - twisAddr);
            jsvUnLock2(i2c,buf);
          }
        }
        break;
>Case TWIS_EVT_READ_REQ 63, 1
read { "addr": 63, "length": 1 } 3f
>Case TWIS_EVT_READ_REQ 64, 0
gfwilliams commented 4 years ago

Oh right - well there's only a 64 byte buffer, so that's all you get.

I2C1.setup(...);
I2C1.buffer = new Uint8Array(256);

should fix it

MaBecker commented 4 years ago

The larger buffer is not fixing it.

adding twisAddr %= twisAddr; will fix it and just can handle hours of calling i2cdetect

    case TWIS_EVT_READ_REQ:
        if (p_event->data.buf_req) {
          JsVar *i2c = jsvObjectGetChild(execInfo.root,"I2C1",0);
          if (i2c) {
            JsVar *buf = jsvObjectGetChild(i2c,"buffer",0);
            size_t bufLen;
            char *bufPtr = jsvGetDataPointer(buf, &bufLen);
            twisAddr %= bufLen;    // <--wrap around
            if (bufPtr && bufLen>twisAddr)
              nrf_drv_twis_tx_prepare(&TWIS1, bufPtr + twisAddr, bufLen - twisAddr);
            jsvUnLock2(i2c,buf);
          }
        }
        break;

What do you think about this wrap around solution?

gfwilliams commented 4 years ago

Hmm, that's an odd one. It's hard to see why the larger buffer wouldn't fix it :(

The problem with the wrap-around solution is... why? I mean, it's almost certainly not what you'd want. I find it very hard to think of a point where you'd want your read to wrap. It's probably better that it errors and then you figure out why your software is trying to read past the end of the buffer. Also, I'm not sure it will read - what if twisAddr is 60, bufLen is 64, but then you want to read 10?

What does watch -n 1 i2cdetect -y -a -r 1 do? Does it actually write a one byte address first? That's the idea - that to read - like with most other I2C devices - you write a one byte register address, then read however many bytes you want.

MaBecker commented 4 years ago

What does watch -n 1 i2cdetect -y -a -r 1 do?

Bildschirmfoto 2020-09-29 um 10 03 26

Does it actually write a one byte address first?

  • hopefully not, because this will change a value no, it does not.
  • only reads like i2cget make sense to detect a device

Even with a larger buffer like 256, nrf_drv_twis_tx_prepare() is not called because of the if condition and that is causing a i2c-bus-blocking situation.

            if (bufPtr && bufLen>twisAddr)
              nrf_drv_twis_tx_prepare(&TWIS1, bufPtr + twisAddr, bufLen - twisAddr);

Possible ways to fix this could be:

what if twisAddr is 60, bufLen is 64, but then you want to read 10?

First of all: This case is buffer size independent. This case should cause a error or just return, as in this case, four bytes.

gfwilliams commented 4 years ago

Does it actually write a one byte address first? ... hopefully not, because this will change a value.

That's not the case though - the first byte you write sets the address.

If you look at pretty much any I2C device it works the same way: http://www.espruino.com/modules/INA226.js

INA226.prototype.rd = function(a) {
  this.i2c.writeTo(this.addr,a);
  var d = this.i2c.readFrom(this.addr,2);
  ....
};

You write a one byte address, then read the data you want. You never issue a read on its own without writing first, so I'm not sure this is something we should really be trying to handle.

I feel like a lot of other I2C devices would behave exactly the same way in this case. I don't think it's a bug.

MaBecker commented 4 years ago

just updated the text - we came over cross ..

MaBecker commented 4 years ago

I feel like a lot of other I2C devices would behave exactly the same way in this case. I don't think it's a bug.

I do not think it's a bug, it's just something that needs to be handled.

gfwilliams commented 4 years ago

Sorry - not my intention at all...

Does it need to be handled though? I mean, if you're doing repeated reads with no write then it's probably a software bug. Espruino shouldn't really have to work in that case - in fact if it keeps working and is used like that it's probably going to lead to errors down the line.

Or are you saying that in this case the nRF52 actually blocks the I2C bus and it can no longer be used for communicating with any other devices?

MaBecker commented 4 years ago

Or are you saying that in this case the nRF52 actually blocks the I2C bus and it can no longer be used for communicating with any other devices?

Yes, this is exactly what happens

gfwilliams commented 4 years ago

Ok, lets fix this then... What happens if you do:

if (bufPtr && bufLen>twisAddr)
  nrf_drv_twis_tx_prepare(&TWIS1, bufPtr + twisAddr, bufLen - twisAddr);
else
 nrf_drv_twis_tx_prepare(&TWIS1, twisRxBuf, 0);

Failing that, we can do:

else {
  memset(twisRxBuf, 255, sizeof(twisRxBuf));
  nrf_drv_twis_tx_prepare(&TWIS1, twisRxBuf, sizeof(twisRxBuf));
}
MaBecker commented 4 years ago

Tested first coding with a 32 byte and 64 byte buffer - works perfekt - Thanks!

Screen shot shows the result of a 32 byte buffer i2c slave behavior.

Bildschirmfoto 2020-09-29 um 13 16 14

gfwilliams commented 4 years ago

You mean using the first option?

MaBecker commented 4 years ago

yes, this one

if (bufPtr && bufLen>twisAddr)
  nrf_drv_twis_tx_prepare(&TWIS1, bufPtr + twisAddr, bufLen - twisAddr);
else
 nrf_drv_twis_tx_prepare(&TWIS1, twisRxBuf, 0);
gfwilliams commented 4 years ago

Great! Thanks for testing - just done!

MaBecker commented 4 years ago
gfwilliams commented 4 years ago

Great! we can close this then?

MaBecker commented 4 years ago

Yes - what a great function - again many thanks!

parasquid commented 4 years ago

Hello! Just wanted to mention that since I was working on the hdc1080 sensor ( https://www.ti.com/lit/ds/symlink/hdc1080.pdf ) on page 14 section 8.6 Register map, it notes that the power on reset pointer is 0x00

Which means that although most sensors would do the "write to I2C address to set the pointer then read" protocol, TI also was expecting use cases where the sensor would have a "default" mode so it can be immediately read without setting the pointer.

Thanks for this! :)

MaBecker commented 4 years ago

Just working on a NRF52840 (SDK15) custom board with i2c slave

targets/nrf5x/jshardware.c: In function 'jshKill':
targets/nrf5x/jshardware.c:861:7: error: implicit declaration of function 'nrf_drv_twis_is_enabled'; did you mean 'nrf_drv_usbd_is_enabled'? [-Werror=implicit-function-declaration]
   if (nrf_drv_twis_is_enabled(TWIS1_INSTANCE_INDEX)) {
       ^~~~~~~~~~~~~~~~~~~~~~~
       nrf_drv_usbd_is_enabled
CC src/jswrap_arraybuffer.o
targets/nrf5x/jshardware.c:861:31: error: 'TWIS1_INSTANCE_INDEX' undeclared (first use in this function); did you mean 'TWIS1_ENABLED'?
   if (nrf_drv_twis_is_enabled(TWIS1_INSTANCE_INDEX)) {
                               ^~~~~~~~~~~~~~~~~~~~
                               TWIS1_ENABLED

.....

targets/nrf5x/jshardware.c: In function 'jshI2CSetup':
targets/nrf5x/jshardware.c:1973:54: error: 'TWIS1_INSTANCE_INDEX' undeclared (first use in this function); did you mean 'TWIS1_ENABLED'?
   if ((device == EV_I2C1) && nrf_drv_twis_is_enabled(TWIS1_INSTANCE_INDEX)) {
                                                      ^~~~~~~~~~~~~~~~~~~~
                                                      TWIS1_ENABLED

OK, twis_slave is missing for SKD15, for first tests took a copy of SDK12 and of cause now the linker is complaining because the missing libs.

uino/targets/nrf5x/jshardware.c:1930: undefined reference to `nrf_drv_twis_rx_prepare'
collect2: error: ld returned 1 exit status
make: *** [espruino_2v08.82_nrf52840_custom.elf] Error 1
gfwilliams commented 4 years ago

I think you just need to edit the targetlibs/..15/nrf_config/sdk_config.h file to enable TWIS1 - you'll probably see the same changed You sure you copied the -DTWIS_ENABLED=1 -DTWIS1_ENABLED=1 lines into the board decl?

Maybe check the twis file gets built: https://github.com/espruino/Espruino/commit/77a60d935908c8837465a59cbb905407ec2e47b4#diff-600fbc2b88437b719bd2062e306061338736300b6e7560a36e96fda185185436R168

As far as I can see though the TWIS stuff is designed for SDK15, so it should work fine.

I guess the obvious question is: You had this working ok - so what changed? Can you still build it for the 52832 ok?

MaBecker commented 4 years ago

I think you just need to edit the targetlibs/..15/nrf_config/sdk_config.h file to enable TWIS1

Hmm, isn't that what those define do?

     # i2c slave
     'DEFINES +=-DI2C_SLAVE -DTWIS_ENABLED=1 -DTWIS1_ENABLED=1', # enable I2C slave support

I guess the obvious question is: You had this working ok - so what changed? Can you still build it for the 52832 ok?

Yes 52832 still build without err eg for PUCKJS_CUTOM.py ( SKD12 )

     # i2c slave
     'DEFINES +=-DI2C_SLAVE -DTWIS_ENABLED=1 -DTWIS1_ENABLED=1',  # enable I2C slave support
MaBecker commented 4 years ago

Just remove nrf5x_15/componentes and other stuff and callled source scripts/provision.sh NRF52840DK to reinstall it.

Same error, just check the zip file, it contains not twis_slave folder ....

Note: nrf5x_15 is not part of the repository, but nrf5x_12, is that why twis_slave folder and libs are not missing?

MaBecker commented 3 years ago

Well I would say with SDK15 there are some major changes starting with location and continue with different structure and function names.

Bildschirmfoto 2020-12-01 um 22 34 52

gfwilliams commented 3 years ago

It's pretty standard for Nordic to randomly rename everything for no reason between literally every bloody SDK version.

I'd be pretty sure this will still be fine when you figure out the correct files to add though. Usually I just see what the linker complains about not having, then grep through the SDK to find out where those functions are defined.