TwiMaster rework

[Riksu9000]

Verification

• [X] I searched for similar issues and found none was relevant.

Introduce the issue

A single TwiMaster instance is created globally in main.cpp, passed to multiple drivers, and the drivers must call the functions with the correct address as an argument. Instantiating multiple TwiMaster will probably break everything.

Preferred solution

TwiMaster could be split into two classes. One handles interfacing with the hardware, and one is instantiated in each driver with the hardware class and address passed to the constructor.

Version

No response

[Riksu9000]

Aren't there TWI drivers in the nrf sdk? Why aren't they used @JF002 ?

[JF002]

TwiMaster could be split into two classes

We could probably do something similar to SpiMaster and Spi: one class to implement the low level driver, and the second one to encapsulate the device address.

Aren't there TWI drivers in the nrf sdk? Why aren't they used @JF002 ?

In the past, we had many issues with the SPI and TWI peripherals : sometimes they would not initialize, go to sleep, work properly, and sometimes they would just hard lock the system. At some point, we didn't know if those issues were coming from the HW or from the drivers. Since we did not need all the complexity and functionalities from the NRF driver, I simply decided to reimplement the low-levels drivers from scratch so the implementation matches exactly what we need in InfiniTime and to make debugging easier.

[Riksu9000]

I don't know about that. Maybe I should've just said that the code is frankly very hard to make sense of. I don't know the specifics of the issue, but I think by using the nrf drivers the code could be much more expressive, and probably less likely to have bugs in it. Either that or improving the readability of the TwiMaster code could resolve this issue.

[Riksu9000]

9ac4be8b759bb2cedeb999ce5e87d983261beded This commit replaced the current code with nrf drivers, but a few days later this commit only partially reverted the changes 012c246e401c0745d4b6765217ce7137680070da. The PR says that "it fixes problems". What happened? This left the twiMaster resetting code in SystemTask, and in my quick testing everything works just fine without it.

[JF002]

I unfortunately do not remember all the details... But I remember spending hours and days trying to debug those weird issues with the bus locking, returning random data, or just freezing for some reasons. Basically, InfiniTime couldn't initialize the touchpanel, hr sensor or motion sensor, or couldn't suddenly read/write on the bus, it would then enter in an infinite loop and just reset.

At some points, I remember I was just fighting against the implementation of the NRF drivers, so I decided to implement the driver from scratch using the reference manual and the errata sheet, so that I could make sense of what was happening on the bus.

The NRF driver implements most of the functionalities available on the TWI bus, while the driver in InfiniTime is fine tuned for our specific hardware and it mostly works.

I don't know what was the root cause of those issues (software or hardware ?) so I can't say for sure if switching back to the NRF driver will work for everyone, unfortunately.

Since I don't want to spend so much time debugging strange issues again, I would personally want to apply the saying "if it ain't broke don't fix it". I'm however OK with refactoring the current driver to improve the readability.

[Riksu9000]

1705 This is what will happen if we don't use nrf hal. One of the top core principles is reliability. If we truly stand by it, we need to write better code. What about the TwiMaster resetting code in SystemTask? I don't feel confident about the reliability of the system if we don't know if that line is important and are afraid to touch it. If there truly is some hardware issue, it must be documented and verifiable, so we are able to make changes without being afraid of breaking everything.