Skip to content

Add WDT Library #242

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.

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

Merged
merged 24 commits into from
Aug 29, 2020
Merged

Add WDT Library #242

merged 24 commits into from
Aug 29, 2020

Conversation

adamgarbo
Copy link
Contributor

Hi @oclyke,

After our recent discussions regarding #237 by @stephenf7072 , I was motivated to get the ball rolling with writing a WDT library the Apollo3 Core. I started working with the WDT HAL earlier this year, and have been meaning put a library together for some time.

Library summary:
For the most part, this library is simply a wrapper of the Ambiq HAL. As we know, the HAL can be scary, so the watchdog functions can now be called using basic commands such as start, stop, restart, etc.

Where I needed to bypass the HAL and switch to writing registers directly was with the watchdog configuration functionality. Normally, when configuring the WDT with the HAL, you need to create a specific am_hal_wdt_config_t structure, specify your clock divider and tick values and then pass this structure to the HAL function am_hal_wdt_init, which actually does the configuration. Rather than dealing with structures, addresses and pointers, I opted to write a few of simple functions (e.g. configure(), setClock(), setInterrupt(), setReset(), to have greater overall control of these registers.

Default configuration: There is a globally declared am_hal_wdt_config_t watchdogConfig structure in the header file, which is used for the initial WDT configuration. This defaults the LFRC clock divider to 16 Hz, the interrupt period to 4 seconds and reset period to 8 seconds. For users of either Atmega328p or SAMD21 watchdogs, these periods will be familiar, though are ultimately arbitrarily and able to be changed.

All examples tested with a SparkFun Redboard Artemis. A test program (not included) was also run to iterate the WDT through every clock divider, interrupt and reset tick combination to ensure correct timings.

My hope is that other users will identify additional required functionality or make contributions themselves. I welcome your feedback!

Cheers,
Adam

Examples:

1. Example1_WDT_Basic

  • An example demonstrating the basic use of the watchdog timer. It will demonstrate to the user watchdog interrupt and reset triggers. Based on the Ambiq HAL watchdog.c example.

2. Example2_WDT_Config

  • A more advanced version of Example 1, which demonstrates how to manually configure the watchdog timer clock divider, interrupt ticks and reset ticks. Comments provide examples on how to calculate the desired interrupt and/or reset period.

3. Example3_WDT_LowPower

  • A more advanced example, which makes use of both the Artemis RTC and WDT libraries. This example demonstrates how to use the WDT in conjunction with RTC deep sleep. Draws from RTC example Example6_LowPower_Alarm.

nseidle and others added 22 commits June 26, 2020 13:51
Bringing up to date with release1.1.2
I hate to undo even a little bit of Adam's good work, but in the interests of a robust core, I think it's best the references to time.h are removed, and the Epoch set/get along with it.

RTC.cpp included <time.h>, which caused a compilation error when these are also included in code:
#include <Time.h>
#include <TimeLib.h>

Some possible issues with Window lack of case sensitivity, refer here: https://forum.arduino.cc/index.php?topic=451360.0

Also with time.h (or Time.h?) being an optional extra install library to the Arduino IDE, best to leave it out of the core, especially if it can cause compilation errors.
this keeps room around for the legacy BLE led example
@adamgarbo
Copy link
Contributor Author

A quick note that the failed checks are due to: fatal error: WDT.h: No such file or directory

Is this perhaps due to using #include <WDT.h> instead of #include "WDT.h" in the examples?

@stephenf7072
Copy link
Contributor

So yes, I've been through and commented, I thought that was just on the files, but it's made a whole lot of comments on the main conversations - any Github tips?

Basically - excellent work, I only had suggestions for:

  • Add a call to am_hal_interrupt_master_enable()? (although perhaps it isn't necessary)
  • Add error checking for wrong values passed into setting the clock frequency
  • Add excessive comments about what can be passed into setting the clock frequency

Added simplied defintions and error checking to setClock() and configure(). More detailed comments.
@adamgarbo
Copy link
Contributor Author

Thanks @stephenf7072!

I believe I've addressed all of your comments in 38a241b. Also, please see your individual comments for more detailed information of specific issues.

Cheers,
Adam

@stephenf7072
Copy link
Contributor

Nice one Adam. I've been over the new bits and apart from the minor omission of a "HZ" in keywords, no further comments from me.

@oclyke
Copy link
Contributor

oclyke commented Aug 19, 2020

Hi @adamgarbo and @stephenf7072

Thanks for all your effort! You both probably know that we are in the midst of a big transition to the v2.0.0 core. That being said I think this PR should be merged into v2.0.0 rather than trying to squeeze it in on the tail end of v1.x.x

This PR is not forgotten!

@adamgarbo
Copy link
Contributor Author

Thanks @oclyke

Is there a way to redirect which branch this PR will merge with? Please let me know if there's anything I can (or should) do.

Also, would you like me to create a new RTC library PR for v2.0.0 with the new time variable order (i.e. hund, sec, min, hour, etc.)?

Cheers,
Adam

@oclyke
Copy link
Contributor

oclyke commented Aug 19, 2020

@adamgarbo I have already put the RTC library w/ the order changes into the v2.0.0 branch (took the liberty to do so cause I didn't want to bug anyone for help with it)

Yes, I will change the target of this PR - then perhaps I can get around to testing it and make it part of v2.0.0 rather than 2.x.x

Edit: actually I think you need to change the target branch. Should be fairly simple / intuitive.... lmk if you have trouble

@adamgarbo adamgarbo changed the base branch from master to v2.0.0 August 19, 2020 18:29
@adamgarbo
Copy link
Contributor Author

@oclyke Well, that was easy! Updated base branch to v2.0.0. I assumed that was the correct one, and not the preview?

@oclyke
Copy link
Contributor

oclyke commented Aug 19, 2020

Yup! There is a sort of rolling release going on like this:

  • develop stuff in v2.0.0
  • merge into spoof-dev to trigger CI builds
  • pull changes and merge spoof-release-candidate into v2.0.0 to be back on track
  • make a snapshot of v2.0.0 in v2.0.0-preview for people to use w/o my changes breaking everything

By the way - there will be a decent amount of work to do to make this compatible with v2.0.0 - there is a completely different file structure and philosophy. I'll be here to lend a hand

@adamgarbo
Copy link
Contributor Author

Thanks, Owen.

Is there any documentation on the new file structure and philosophy? Keen to learn more and happy to make changes as necessary.

Cheers,
Adam

@oclyke
Copy link
Contributor

oclyke commented Aug 19, 2020

A quick glance at the new diffs makes me think..... it would probably be easiest to start from scratch based on the v2.0.0 branch. You could of course make a local copy of the meat + potatoes files of your PR.

@oclyke
Copy link
Contributor

oclyke commented Aug 19, 2020

Wow - you're quick. Didn't see your last request for docs.

I don't have anything official to hand you. Maybe you will be able to tell just by looking at the file structure. Also possible that you won't be affected if you are just writing a library. Here are some notes:

  • run git submodule update --init --recursive1 in the core repo to make sure all the submodules exist for you

  • standard Arduino API functions that can be handled by mbed go in cores/arduino/mbed-bridge (which is its own repo)

  • standard Arduino API functions that rely on the Ambiq HAL go in cores/arduino/sdk

  • generally I am matching implementation files with their respective header files - even when it means having two c or cpp files with the same name in different core locations

  • license notices are shortened and just point at the LICENSE.md file in the core

There's probably a lot more - that's all for now though

@oclyke oclyke changed the base branch from v2.0.0 to release-candidate August 27, 2020 18:42
@oclyke
Copy link
Contributor

oclyke commented Aug 27, 2020

@adamgarbo - we've decided to do another support release on the v1.x core, so I will test this out and try to pull it in. That way we can have a reference for what we should be doing on the v2.0.0 version

@adamgarbo
Copy link
Contributor Author

adamgarbo commented Aug 27, 2020

Hi @oclyke,

Sounds good! Please let me know if you need any assistance or clarification.

I haven't yet had a chance to rewrite the WDT library, but if the v2.0.0 RTC library is any example, it shouldn't be too much work. I noticed some of the changes you mentioned (e.g. #ifndef _APOLLO3_LIBRARIES_RTC_H_ and class Apollo3RTC {}).

I'll try to submit a PR to v.2.0.0 in the next couple of days. What's your optimistic timeframe for its release?

Cheers,
Adam

@oclyke
Copy link
Contributor

oclyke commented Aug 27, 2020

@adamgarbo I just tried out these examples. First of all (I hope I have said this before) thanks for the great work! Your examples are well laid out and straight to the point.

The first example worked well. I then went on to test examples 2 and 3, but saw no output. Stumped, I went back to example 1 (basic) and saw no output any more. Moving on I even went to an unrelated example (pure serial) I still saw no output.

I haven't looked into it any deeper (using a debugger, for example) but can you confirm that you do not have similar issues?

@adamgarbo
Copy link
Contributor Author

Hi Owen,

I initially tested all examples on a SparkFun Artemis Redboard. I just now tested them on an Edge 2 and they all appeared to be working correctly. Please see below for their respective outputs:

Example 1

15:18:15.045 -> Artemis Watchdog Timer Example
15:18:19.215 -> Interrupt: 1 Period: 4122 ms 
15:18:23.277 -> Interrupt: 2 Period: 4067 ms 
15:18:27.386 -> Interrupt: 3 Period: 4065 ms 
15:18:31.429 -> Interrupt: 4 Period: 4067 ms 
15:18:35.523 -> Interrupt: 5 Period: 4064 ms 
15:18:35.523 -> Warning: Watchdog has triggered a system reset

Example 2

15:18:55.097 -> �Artemis Watchdog Timer Example
15:18:56.357 -> Interrupt: 1 Period: 1007 ms
15:19:00.393 -> Interrupt: 2 Period: 4019 ms
15:19:04.480 -> Interrupt: 3 Period: 4066 ms
15:19:08.549 -> Interrupt: 4 Period: 4066 ms
15:19:12.639 -> Interrupt: 5 Period: 4068 ms
15:19:16.714 -> Interrupt: 6 Period: 4064 ms
15:19:20.790 -> Interrupt: 7 Period: 4064 ms
15:19:24.855 -> Interrupt: 8 Period: 4065 ms
15:19:28.954 -> Interrupt: 9 Period: 4065 ms
15:19:28.954 -> Warning: Watchdog has triggered a system reset

Example 3

15:20:16.623 -> �Artemis Watchdog Low Power Example
15:20:16.877 -> ⸮Watchdog interrupt: 2020-08-01 00:00:10.11
15:20:27.015 -> ⸮Watchdog interrupt: 2020-08-01 00:00:20.15
15:20:37.042 -> ⸮Watchdog interrupt: 2020-08-01 00:00:30.20
15:20:47.074 -> ⸮Watchdog interrupt: 2020-08-01 00:00:40.26
15:20:57.122 -> ⸮Watchdog interrupt: 2020-08-01 00:00:50.31
15:21:07.192 -> ⸮Alarm interrupt: 2020-08-01 00:01:00.01
15:21:16.872 -> ⸮Watchdog interrupt: 2020-08-01 00:01:10.04

I could have sworn we fixed that issue with the Artemis going to sleep and waking up without producing a question mark in the serial output. @nseidle did we only fix this on the OpenLog Artemis?

Cheers,
Adam

@oclyke
Copy link
Contributor

oclyke commented Aug 27, 2020

Thanks - I will look more closely at my setup to see if something is going wrong.

I also thought we had fixed the UART startup issue. If I recall correctly it had to do with configuring the UART pins after the UART was initialized...

@oclyke oclyke merged commit 1b8de22 into sparkfun:release-candidate Aug 29, 2020
@oclyke
Copy link
Contributor

oclyke commented Aug 29, 2020

Alright, I think I found the issue and it is totally unrelated to this library. It is described in issue #254

I don't believe this is at all related to this PR, and it doesn't occur with the SVL loader either (hence why most people are unlikely to experience the issue)

@adamgarbo adamgarbo deleted the wdt branch August 29, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants