Skip to content

TwiMaster rework #568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
JF002 merged 6 commits into from
Aug 28, 2021
Merged

TwiMaster rework #568

JF002 merged 6 commits into from
Aug 28, 2021

Conversation

@Riksu9000
Copy link
Contributor

@Riksu9000 Riksu9000 commented Aug 10, 2021 •
edited
Loading

Made using TwiMaster simpler. The bus is simply enabled on each read/write and disabled when done. There's no need to think about or control the state outside of TwiMaster. I don't know if this is more expensive, but it seems to work just fine.

Internal pullups were enabled. They're unnecessary because external pullups are used.

This will fix the freezing issue in #492 so I can finally move forwards with it.

Sleep() and Wakeup() can almost be made private, but there's still this piece of code in SystemTask I'm not brave enough to touch.

  // Reset the TWI device because the motion sensor chip most probably crashed it...
  twiMaster.Sleep();
  twiMaster.Init();

@Riksu9000 Riksu9000 mentioned this pull request Aug 10, 2021
Merged
@Riksu9000
Copy link
Contributor Author

Riksu9000 commented Aug 10, 2021

Does disabling the bus or changing pin configuration actually save power?

@JF002
Copy link
Collaborator

JF002 commented Aug 11, 2021

Does disabling the bus or changing pin configuration actually save power?

I think I found some info in datasheet/nrf devzone saying that the peripherals (spi, twi,...) would consume a bit of current when enabled (even with no communication happening). They also keep the HFClock enabled. That's why I tried to disabled them as soon as the watch goes to sleep mode.
I'll try to find these info again ;)

JF002
JF002 reviewed Aug 14, 2021
View reviewed changes
src/drivers/TwiMaster.cpp Outdated
// TODO use DMA/IRQ

TwiMaster::TwiMaster(const Modules module, const Parameters& params) : module {module}, params {params} {
mutex = xSemaphoreCreateBinary();
Copy link
Collaborator

@JF002 JF002 Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initialization of the mutex was previously done in the ctor(), but I moved it to Init() in 6d524eb. The goal was to avoid Static Initialization Order Fiasco (see this.
I also wanted to ensure FreeRTOS was correctly initialized before instantiating mutexes and other objects.

Can we ensure this won't cause any issue?

Copy link
Contributor Author

@Riksu9000 Riksu9000 Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a separate function called xSemaphoreCreateBinaryStatic(), so I'd assume that one isn't static and works correctly.

Copy link
Contributor Author

@Riksu9000 Riksu9000 Aug 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I completely understand the issue, but this doesn't access global state so it doesn't apply right? I dont think we shoud be afraid of using constructors either because they can make the classes safe to use right away, and I would even like to move more stuff to the constructors to reduce the Init functions in code that can be avoided.

Copy link
Collaborator

@JF002 JF002 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course, most of the time, we shouldn't need an Init() function and use the ctor() to initialise the whole object. However, in this case, the ctor is called by the Startup code, before main() is called.
When you look at the code of xSemaphoreCreateBinary() you see it access to variables from FreeRTOS and even calls pvPortMalloc().

I'm not sure completely sure that it's ok that call freertos function before the scheduler is init'd and running.
It might work, as previously, all mutexes were initialized in the ctro(), but I moved them when I was chasing memory corruption issue (which, in the end, were not caused by those initializations in the ctor() so...).

Copy link
Contributor Author

@Riksu9000 Riksu9000 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it back so we don't need to worry about it now.

@Riksu9000
Copy link
Contributor Author

Riksu9000 commented Aug 16, 2021

I found something weird.

static constexpr uint32_t MaxTwiFrequencyWithoutHardwareBug {0x06200000};
This is set as the frequency parameter

But then it is cast to Frequencies. What will happen here?

enum class Frequencies { Khz100, Khz250, Khz400 };

  switch (static_cast<Frequencies>(params.frequency)) {
    case Frequencies::Khz100:
      twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K100;
      break;
    case Frequencies::Khz250:
      twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K250;
      break;
    case Frequencies::Khz400:
      twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K400;
      break;
  }

The attempted frequency parameters value is slightly smaller than the K400 value, but can we be sure its safe because its not documented?

#define TWIM_FREQUENCY_FREQUENCY_K100 (0x01980000UL) /*!< 100 kbps */
#define TWIM_FREQUENCY_FREQUENCY_K250 (0x04000000UL) /*!< 250 kbps */
#define TWIM_FREQUENCY_FREQUENCY_K400 (0x06400000UL) /*!< 400 kbps */

@JF002
Copy link
Collaborator

JF002 commented Aug 18, 2021

Indeed, this casting for the frequency does not look right.
Iirc, I had to use that MaxTwiFrequencyWithoutHardwareBug constant to fix an issue reference in the errata sheet of the mcu.

@Riksu9000
Copy link
Contributor Author

Riksu9000 commented Aug 18, 2021

Iirc, I had to use that MaxTwiFrequencyWithoutHardwareBug constant to fix an issue reference in the errata sheet of the mcu.

Okay I found the same issue mentioned there and the same workaround so it seems fine to use.

@JF002
Copy link
Collaborator

JF002 commented Aug 18, 2021

But I think this cast and switch/case should be fixed, because, in the end, the workaround value is not written to FREQUENCY.

@Riksu9000
Copy link
Contributor Author

Riksu9000 commented Aug 18, 2021

But I think this cast and switch/case should be fixed, because, in the end, the workaround value is not written to FREQUENCY.

I already fixed it :)

@JF002
Copy link
Collaborator

JF002 commented Aug 18, 2021

I already fixed it :)

Awesome, you're too fast for me ! :-D

@JF002 JF002 added this to the Version 1.4 milestone Aug 18, 2021
@Riksu9000 Riksu9000 mentioned this pull request Aug 19, 2021
Merged
@JF002 JF002 merged commit 45e7638 into InfiniTimeOrg:develop Aug 28, 2021
@Riksu9000 Riksu9000 mentioned this pull request Oct 8, 2021
Closed
@Riksu9000 Riksu9000 deleted the twimaster_rework branch October 23, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JF002 JF002 JF002 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Version 1.4

Development

Successfully merging this pull request may close these issues.

2 participants

@Riksu9000 @JF002