You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/01/13 10:51:28 UTC
[GitHub] [incubator-nuttx] raiden00pl opened a new pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
raiden00pl opened a new pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674
## Summary
Add support for I2C_M_NOSTOP and I2C_M_NOSTART flags.
NRF52 hardware doesn't allow direct control of START/STOP bits, so the only solution is to combine messages into one.
## Impact
## Testing
Tested with SSD1306 and XX24XX EEPROM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759639786
> It looks like this can be implemented by hooking into SUSPEND which would give full control of start/stop.
I also tried this approach but without success :)
You can stretch the transmission with SUSPEND/RESUME without emitting a START byte, but this works only for currently processed data chunk. If you try to update PTR or MAXCNT after SUSPENDED event, you have to call STARTTX and emit a START byte again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759585524
Repeated stop must be specified in the flags that _is_ the configuration feature we should not be hiding this behind more flags. If we detect that there is not repeated start then we should be able to just use the DMA list feature correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759572116
I don't see how "list mode" can help in this case.
Each TASKS_STARTTX trigger causes a START byte to be sent. EasyDMA can only update the data pointer (TXD.PTR), but we still need to trigger TASKS_START to send data.
We need contiguous buffer in memory to send it with a single TASKS_STARTTX to prevent HW from sending multiple START bytes between messages.
Agree it looks bad but unfortunately I haven't found any other working solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761846692
Ok, thanks for that link. It seems it is a limitation that is not evident from the documentation. I suppose that we will have to live with this copy.
Still, I'm not sure if the condition (either NOSTART or NOSTOP) is correct, as I think the case that we need to condition it on is only NOSTART. NOSTOP without NOSTART should generate a repeated start and would thus not require the copy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761846166
> But my point still stands, we should be able to update the pointer/size and use suspend/resume to achieve the desired effect of not sending the stop/start.
As I said before it doesn't work that way.
Here, someone had a similar problem for the nRF52 SDK:
https://devzone.nordicsemi.com/f/nordic-q-a/66992/resume-suspended-twim-transmission-on-nrf52
The only option is to combine the messages into a single buffer and there's not much we can do about it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761842171
> I looked at TWI (not TWIM) docs and indeed repeated starts are supported (I understood from the conversation that this wasn't supported by TWI). So you're right we don't need DMA for this, although it would be nice to have. I don't think there's a limitation on message length since the pointer and data size can be updated on the fly.
Please note that we are using the TWIM interface, not the TWI interface.
TWIM *always* uses EasyDMA. So the question is not if we need EasyDMA but if we need EasyDMA array list (TXD.LIST=1). My point is, we don't need EasyDMA list because it doesn't give us anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761760566
>
> 2. In my experience, this is not a rarely required feature, but quite the opposite. We should be careful with making decisions based on our subjective experiences as they can vary greatly.
>
Most of the I2C device drivers use this mode for reading registers, so we have a byte in msg 1 and then the read buffer. I'm trying to understand how this works today, are we doing a repeated start? Devices are certainly working today on i2c with the nrf52 and I recall the signal looking correct for the transfer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761854183
Unfortunately, they did not work and it took me a lot of nerves to find the cause ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761619149
So, to sum it up I believe the situation is as follows:
1. we should support repeated start using list feature (this is also good in terms of using DMA for large transfers)
2. for transmissions involving multiple messages where no stop nor repeated start should be sent, copying everything to one chunk only seems possible.
I don't really like to do 2. under the hood as it could involve a lot of data being allocated copied without user knowledge. what if we simply don't support this considering it as a hardware limitation? As discussed I think this is rarely required as in most cases the correct approach is to use a repeated start. This may require looking at existing drivers to see if they actually need to be changed to use a repeated start (assuming the device supports it) instead of just expecting one whole transaction.
I think either way 1. could be separately addressed as it may touch a bunch of code. @raiden00pl would you be interested in doing that? I'm currently using I2C device on nRF52 so I could give it a try if not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761843052
But my point still stands, we should be able to update the pointer/size and use suspend/resume to achieve the desired effect of not sending the stop/start.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761881757
> > I am wondering if it would be worth keeping a small buffer around 2-4 bytes that we could use instead of malloc free all of the time. This would save a lot of overhead for small transfers.
>
> I was thinking about something similar.
> For NRF52832 we could even keep a buffer with the maxium supported length = 255.
> But for NRF52840 it doesn't make sene because the maximum length of sent data can be 65535.
> Another option is a buffer with a user-configurable length, and I think that's a pretty reasonable solution.
I was thinking we keep it short and fall back on malloc because I would expect the large writes are usually going to be constrained to displays and memory devices which are already going to be slow on i2c and also tend to use more ram than say a small sensor. But I would be fine making it configurable with a default around 4 bytes which would cover 2 byte reads to a 16bit "address". Do you want me to create a PR for this after we merge this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759464197
I don't think that this solution is very good, doing this kind of allocation and copy can be quite wasteful depending on message size. I think it is possible to do this by using "list mode" DMA (see SPI implementation on how this is used to transfer arbitrarily sized data in chunks, as one large transaction). For reference see p309 of nRF52832 PS.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759633718
>
> > NOSTOP is implied in the first message if the NOSTART flag is set on the second one. Drivers are inconsistent about it they set that flag on the first message or leave it 0 but this is what our interface contract says.
>
> If by NOSTOP you mean repeated start then no, NOSTART just means that the following message(s) are a continuation. There is no need to reconfigure and no need to send a repeated start. It's just a bunch of bytes read or written one after the other.
What I am saying is this:
```
* msg[n].flags msg[n+1].flags Behavior
* ------------ --------------- -----------------------------------------
* 0 0 Two normal, separate messages with STOP
* on msg[n] then START on msg[n+1]
* 0* I2C_M_NOSTART Continuation of the same transfer (must
* be the same direction). See NOTE below.
* NO_STOP 0 No STOP on msg[n]; repeated START on
* msg[n+1].
*
* * NOTE: NO_STOP is implied in this case and may or not be explicitly
* included in the msg[n] flags
*/
```
Drivers are not consistent about NO_STOP in the first message (which is fine), but it is implied as is called out in that note and we need to respect that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton merged pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759610698
> In that case, I would at least put this behind a config flag (disabled by default), otherwise a performance penalty will be paid even when not really necessary.
This is a standard feature of the OS so it should be enabled by default. But configuration option to disable additional code is OK.
> If we detect that there is not repeated start then we should be able to just use the DMA list feature correct?
DMA requires TASKS_STARTTX=1 to send the chunks of data, which also sends a START/RESTART byte between chunks, so thise doesn't solve the problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761622236
1. Would be good for places where are doing the repeated start. Happy to test this on my hardware.
2. Would we just assert as not supported in this case? I guess if we can cover all the normal cases maybe we can just have this mode be a default off option, that would give you a low performance escape hatch if needed, but you would be aware. I was trying to remember why this sounded familiar and its because the i2c driver I wrote for the sim under Linux has the same limitation where you end up having to assemble a single buffer for these.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761763912
This is what the write buffer for SSD1306 looks like with the current master:
<img width="1911" alt="2021-01-17_10-45" src="https://user-images.githubusercontent.com/6563055/104836936-9e1ba680-58b1-11eb-845b-93741d9ce2c1.png">
This is what the write buffer with NOSTART flag supported looks like:
<img width="1854" alt="2021-01-17_10-46" src="https://user-images.githubusercontent.com/6563055/104836980-ed61d700-58b1-11eb-8238-7b16b96a93e9.png">
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759608732
> > > I see NOSTART used in various drivers, does that not expect a message without a (repeated) start?
> >
> >
> > It doesn't expect a repeated start. NOSTART just means that the next chunk is a continuation of the old data.
>
> Right, that is what I meant. I was referring to Brennan's comment regarding that none of the drivers used this feature, but I see they actually do so they would all use the memcpy() implementation.
Ah ok. Regarding the NOSTOP flag, yes, that's what makes the driver send a repeated start. However, if my recolation is right some drivers (stm32 I believe) do send a repeated start without the NOSTOP flag. The PIC32MZ driver needs NOSTOP to send a rpeated start.
That said not all drivers use a repeated start, most of them do a stop (completely relinquish the bus) then a start again.
An example of drivers using NOSTOP: https://github.com/apache/incubator-nuttx/blob/master/drivers/timers/mcp794xx.c#L298
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761753545
1. I still don't get why we should use the DMA list anywhere. Repeated start works by default and there is no problem with that. We trigger STARTTX for each I2C message, so new START is sent. The DMA list can be useful when all outgoing messages have the same length. But that is not our case.
Anyway, what this DMA mode does is just automatic data PTR incrementing, nothing more.
2. In my experience, this is not a rarely required feature, but quite the opposite. We should be careful with making decisions based on our subjective experiences as they can vary greatly.
This feature should be supported in some way. Getting rid of it completely is the worst option.
I still think it should be on by default, but I won't insist. My reasoning is as follows.
1. The NOSTART/NOSTOP flags are the basic functionality required by the I2C interface from the I2C lower-half implementation.
2. Disabling support for them is only an additional optimization of the system.
3. Basic system features should be enabled by default.
4. Additional system optimizations should be disabled by default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759628332
> That driver looks a little suspect to me usually you so not want a STOP in the register read.
That's not a STOP, that's a repeated start. The chain is: start - write (set reg to read from) in this case - repeated start - read - stop.
> NOSTOP is implied in the first message if the NOSTART flag is set on the second one. Drivers are inconsistent about it they set that flag on the first message or leave it 0 but this is what our interface contract says.
If by NOSTOP you mean repeated start then no, NOSTART just means that the following message(s) are a continuation. There is no need to reconfigure and no need to send a repeated start. It's just a bunch of bytes read or written one after the other.
(getting a bit out of topic for this PR, sorry!)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759622135
Taking another look here
https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.nrf52832.ps.v1.1%2Ftwim.html
It looks like this can be implemented by hooking into SUSPEND which would give full control of start/stop.
The EasyDMA is not clear to me that you would get the stop start since it says the prt and maxcnt are double buffered and updated soon as RX/TXSTARTED
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761881757
> > I am wondering if it would be worth keeping a small buffer around 2-4 bytes that we could use instead of malloc free all of the time. This would save a lot of overhead for small transfers.
>
> I was thinking about something similar.
> For NRF52832 we could even keep a buffer with the maxium supported length = 255.
> But for NRF52840 it doesn't make sene because the maximum length of sent data can be 65535.
> Another option is a buffer with a user-configurable length, and I think that's a pretty reasonable solution.
I was thinking we keep it short because I would expect the large writes are usually going to be constrained to displays and memory devices which are already going to be slow on i2c and also tend to use more ram than say a small sensor. But I would be fine making it configurable with a default around 4 bytes which would cover 2 byte reads to a 16bit "address". Do you want me to create a PR for this after we merge this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761852882
> Still, I'm not sure if the condition (either NOSTART or NOSTOP) is correct, as I think the case that we need to condition it on is only NOSTART. NOSTOP without NOSTART should generate a repeated start and would thus not require the copy.
I agree. NOSTART on the second message should be the only case for copying.
I fixed it and added an option to disable I2C_M_NOSTOP flag support.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759594761
I see NOSTART used in various drivers, does that not expect a message without a (repeated) start? From the i2c header file a repeated start requires a NOSTOP on previous message and a zero flag on following one, so that would be the case of repeated starts, right? The change enables the copy on both cases so I think most drivers will see the copy when they could actually have repeated start in most cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759639786
> It looks like this can be implemented by hooking into SUSPEND which would give full control of start/stop.
I also tried this approach but without success :)
You can stretch the transmission with SUSPEND/RESUME without emitting a START byte, but this works only for currently processed data chunk. If you try to update PTR or MAXCNT after SUSPENDED event, you have to call STARTTX and emit a START byte again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761857129
Ill give this a test today.
I am wondering if it would be worth keeping a small buffer around 2-4 bytes that we could use instead of malloc free all of the time. This would save a lot of overhead for small transfers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759615305
> > > > I see NOSTART used in various drivers, does that not expect a message without a (repeated) start?
> > >
> > >
> > > It doesn't expect a repeated start. NOSTART just means that the next chunk is a continuation of the old data.
> >
> > Right, that is what I meant. I was referring to Brennan's comment regarding that none of the drivers used this feature, but I see they actually do so they would all use the memcpy() implementation.
>
> Ah ok. Regarding the NOSTOP flag, yes, that's what makes the driver send a repeated start. However, if my recolation is right some drivers (stm32 I believe) do send a repeated start when necessary without the NOSTOP flag. The PIC32MZ driver needs NOSTOP to send a repeated start.
> That said not all drivers use a repeated start, most of them do a stop (completely relinquish the bus) then a start again.
> An example of drivers using NOSTOP: https://github.com/apache/incubator-nuttx/blob/master/drivers/timers/mcp794xx.c#L298
That driver looks a little suspect to me usually you so not want a STOP in the register read.
NOSTOP is implied in the first message if the NOSTART flag is set on the second one. Drivers are inconsistent about it they set that flag on the first message or leave it 0 but this is what our interface contract says.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759576858
So a repeated start is not sufficient in general?
In that case, I would at least put this behind a config flag (disabled by default), otherwise a performance penalty will be paid even when not really necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759602281
> I see NOSTART used in various drivers, does that not expect a message without a (repeated) start? From the i2c header file a repeated start requires a NOSTOP on previous message and a zero flag on following one, so that would be the case of repeated starts, right? The change enables the copy on both cases so I think most drivers will see the copy when they could actually have repeated start in most cases.
Sorry yes I was remembering wrong on this. I had to handle this on another i2c master, my point remains though that in this case this should just be treated as a single transfer and that should be fully supported by using the EasyDMA List feature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761863287
> Ill give this a test today.
I just remembered there is something wrong with sending large packets of data for I2C with IRQs enabled (I2C_POLLED=n). After some time, I2C freezes. I haven't diagnosed the problem yet, but I'll let you know if you come across something similar. Polled I2C works fine for me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759602281
> I see NOSTART used in various drivers, does that not expect a message without a (repeated) start? From the i2c header file a repeated start requires a NOSTOP on previous message and a zero flag on following one, so that would be the case of repeated starts, right? The change enables the copy on both cases so I think most drivers will see the copy when they could actually have repeated start in most cases.
Sorry yes I was remembering wrong on this. I had to handling this on another i2c master, my point remains though that in this case this should just be treated as a single transfer and that should be fully supported by using the EasyDMA List feature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759608732
> > > I see NOSTART used in various drivers, does that not expect a message without a (repeated) start?
> >
> >
> > It doesn't expect a repeated start. NOSTART just means that the next chunk is a continuation of the old data.
>
> Right, that is what I meant. I was referring to Brennan's comment regarding that none of the drivers used this feature, but I see they actually do so they would all use the memcpy() implementation.
Ah ok. Regarding the NOSTOP flag, yes, that's what makes the driver send a repeated start. However, if my recolation is right some drivers (stm32?) do send a repeated start without the NOSTOP flag. The PIC32MZ driver needs NOSTOP to send a rpeated start.
That said not all drivers use a repeated start, most of them do a stop (completely relinquish the bus) then a start again.
An example of drivers using NOSTOP: https://github.com/apache/incubator-nuttx/blob/master/drivers/timers/mcp794xx.c#L298
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761853582
Great, thanks.
BTW, did these two devices not work prior to your change? If so, have you tried changing them to use repeated start?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761760566
>
> 2. In my experience, this is not a rarely required feature, but quite the opposite. We should be careful with making decisions based on our subjective experiences as they can vary greatly.
>
Most of the I2C device drivers use this mode for writing registers, so we have a byte in msg 1 and then the write buffer. I'm trying to understand how this works today, are we doing a repeated start? Devices are certainly working today on i2c with the nrf52 and I recall the signal looking correct for the transfer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759586994
In practices none of the drivers in tree use continued messages that do not expect repeated start so I would not expect to see this memcopy flow hit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761859728
> I am wondering if it would be worth keeping a small buffer around 2-4 bytes that we could use instead of malloc free all of the time. This would save a lot of overhead for small transfers.
I was thinking about something similar.
For NRF52832 we could even keep a buffer with the maxium supported length = 255.
But for NRF52840 it doesn't make sene because the maximum length of sent data can be 65535.
Another option is a buffer with a user-configurable length, and I think that's a pretty reasonable solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 edited a comment on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759608732
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761760566
>
> 2. In my experience, this is not a rarely required feature, but quite the opposite. We should be careful with making decisions based on our subjective experiences as they can vary greatly.
>
Most of the I2C device drivers hit use this for reading registers, so we have a byte in msg 1 and then the read buffer. I'm trying to understand how this works today, are we doing a repeated start? Devices are certainly working today on i2c with the nrf52 and I recall the signal looking correct for the transfer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759598913
> I see NOSTART used in various drivers, does that not expect a message without a (repeated) start?
It doesn't expect a repeated start. NOSTART just means that the next chunk is a continuation of the old data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-759600362
> > I see NOSTART used in various drivers, does that not expect a message without a (repeated) start?
>
> It doesn't expect a repeated start. NOSTART just means that the next chunk is a continuation of the old data.
Right, that is what I meant. I was referring to Brennan's comment regarding that none of the drivers used this feature, but I see they actually do so they would all use the memcpy() implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2674: nrf52_i2c: add support for I2C_M_NOSTOP and I2C_M_NOSTART flags
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2674:
URL: https://github.com/apache/incubator-nuttx/pull/2674#issuecomment-761830468
> 1. I still don't get why we should use the DMA list anywhere. Repeated start works by default and there is no problem with that. We trigger STARTTX for each I2C message, so new START is sent. The DMA list can be useful when all outgoing messages have the same length. But that is not our case.
> Anyway, what this DMA mode does is just automatic data PTR incrementing, nothing more.
I looked at TWI (not TWIM) docs and indeed repeated starts are supported (I understood from the conversation that this wasn't supported by TWI). So you're right we don't *need* DMA for this, although it would be nice to have. I don't think there's a limitation on message length since the pointer and data size can be updated on the fly.
> I also tried this approach but without success :)
>
> You can stretch the transmission with SUSPEND/RESUME without emitting a START byte, but this works only for currently processed data chunk. If you try to update PTR or MAXCNT after SUSPENDED event, you have to call STARTTX and emit a START byte again.
I think that is actually the point about using TWIM with its DMA features since they explicitly state that you can update the PTR and MAXCNT as they are double buffered. So to avoid the STOP you would suspend transmission as suggested, update pointer and then resume:
There's actually an example for repeated starts where it shows how to update the PTR/MAXCNT in between:
I think that simply not doing the STARTRX/TX would not send the repeated start.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org