Skip to content

Add variable I2C clock speed support #150

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 4 commits into from
Apr 13, 2020
Merged

Add variable I2C clock speed support #150

merged 4 commits into from
Apr 13, 2020

Conversation

nseidle
Copy link
Member

@nseidle nseidle commented Apr 12, 2020

The HAL only supports Wire.setClock(#) where # must be 100000, 400000, or 1000000. Otherwise, it exists with error and sketches fail to run. This PR:

  • Allows Wire.setClock(123456) to accurately set the I2C hardware to 124kHz. No, for reels:

image

Why?

There may be instance where the user wants to operate outside of the 100/400/1000kHz slots. Mainly, running the bus more slowly minimizes bit errors when the bus capacitance gets large (ie, the user has some really long wires). Or maybe there's an exotic I2C device that only runs at 10kHz.

Min bus speed is now 1500Hz, max speed is 2MHz. Math can be verified here. Testing was done with a RedBoard Artemis and a MS8607 breakout that surprisingly (shockingly) worked at 2MHz and 1500Hz.

This PR was tested with Burst Mode and is unaffected at 96MHz. IOMs seem to run on a separate clock from internal processing.

I am a little concerned that Ambiq makes some very magical recommendations for the TOTPER and FSEL settings for the 3 main I2C speeds. Perhaps they know something about the hardware they are not sharing. So this PR maintains their recommendations, but if the user selects a non-standard I2C bus speed, the hardware will seek it. This PR does not attempt to calculate SDA/SCL end delays.

image

I'm not sure how many modifications we've done to the HAL in the past. What is the best way to document HAL changes so that we can implement our small changes into future SDK version changes? For this reason I've created HAL-Variances.md in the am_sdk_ap3 directory (best place?).

My only real concern with this PR is the hard coding of 48000000 in the loop. I tried to find a good define in the HAL but couldn't come up with one. And we don't want to do a am_hal_clkgen_status_get() because it would change depending on if the user was in burst mode or not.

Here's 1.5kHz:

image

Here's 2MHz:

image

@oclyke
Copy link
Contributor

oclyke commented Apr 13, 2020

Excellent!

We haven't modified the HAL functionality much in the past, and I think it might be best to steer clear of that. Why? Mostly for documentation - although our Git commits will make it possible to recover our fixes when we upgrade the HAL those changes tend to be large and we might miss those differences at first. Then we'd encounter trouble (missing functionality) and then have to dig through the commits to find the difference. Instead I suggest we make a copy of the functionality and maintain it alongside / within our main codebase. I will add some commits to that effect.

This keeps our additional functionallity avaialble in our own source code so that the HAL can be easily updated later w/o worrying about undoing the configuration work.
@oclyke
Copy link
Contributor

oclyke commented Apr 13, 2020

While I was poking around in there I noticed that the SPI half of the configuration function uses iom_get_interface_clock_cfg() to configure the clock speed arbitrarily. Because the IOMaster peripherals operate very similarly in both I2C and SPI modes I think that this function may be all we need - therefore we would not need to re-implement the calculations.

Also, there is this set of macros (which could be used to not hard-code 48000000 in our own codebase)

#define AM_HAL_IOM_MAX_FREQ AM_HAL_IOM_48MHZ
#define AM_HAL_IOM_48MHZ 48000000

@oclyke
Copy link
Contributor

oclyke commented Apr 13, 2020

Alright, so iom_get_interface_clock_cfg() did properly set the frequency but using that method caused the I2C transactions to appear malformed. I think for now we can accept this PR and perhaps come back to it later

@oclyke oclyke merged commit 7706896 into master Apr 13, 2020
@oclyke oclyke deleted the adjustable-I2C-speed branch April 13, 2020 17:12
@oclyke oclyke restored the adjustable-I2C-speed branch April 13, 2020 18:06
@oclyke oclyke deleted the adjustable-I2C-speed branch April 13, 2020 18:07
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.

2 participants