Skip to content

Add setClockDivider. Fix polarity. #30

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

Closed
wants to merge 9 commits into from
Closed

Add setClockDivider. Fix polarity. #30

wants to merge 9 commits into from

Conversation

nseidle
Copy link
Member

@nseidle nseidle commented Jul 10, 2019

One big pitfall was the lack of return AP3_OK from initialize(). The lack of it did not throw an error but caused very weird behavior. Where else do we not have return values?

I've got a request into Ambiq for HAL change to expose SPI LSB/MSB. For now, it's a hard coded register address that should work across all 6 IOM instances.

Nathan Seidle and others added 5 commits July 9, 2019 16:56
We can't put bitOrder in the config structure because the HAL defines am_hal_iom_config_t. So instead we pass it by argument to the two initialize functions. Not elegant but functional.
One big pitfall was the lack of return AP3_OK from initialize. The lack of it did not throw an error but caused very weird behavior. Where else do we not have return values?
@nseidle
Copy link
Member Author

nseidle commented Jul 10, 2019

Note that transfer16 uses bitOrder to indicate byte order. I think this is an error but I'm not sure what you were intending. If transfer16() is a new unique function to Apollo3 I would suggest adding a bool called byteOrder that is passed in rather than relying on global var.

@nseidle nseidle mentioned this pull request Jul 10, 2019
@oclyke
Copy link
Contributor

oclyke commented Jul 10, 2019

Thanks for the PR, looks like a good functionality boost.

Here's where transfer16() came from: https://www.arduino.cc/en/Reference/SPITransfer
The implementation in the SAMD core sends the LS byte first when the bit order is LSB and the MS byte first when the bit order is MSB. Users can call "setBitOrder(BitOrder order)" to permanently change the order for the given SPI port.

Right now this PR breaks "Wire" because both Wire and SPI use the "ap3_iomaster" class, but Wire does not provide the 'bit order' constructor argument. Does the bit order bit apply to both I2C and SPI? If it only applies to SPI then I'd prefer to keep that isolated to the SPI library for now - and ultimately it would be good to get HAL support for the bit order. If not then we can add 'bit order' to the Wire library.

@oclyke
Copy link
Contributor

oclyke commented Jul 10, 2019

Looks like the API for SPILSB is available, so let's use that in the SPI library and keep "iomaster" and "Wire" untouched. How does that sound?

// void setClockDivider(uint8_t uc_div);
void setClockDivider(uint8_t div);

void _transfer(void *buf_out = NULL, void *buf_in = NULL, size_t count = 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not taking this change since '_transfer' is meant to be for internal use of the SPI ibrary

@@ -104,22 +114,22 @@ class SPIClass : public IOMaster {

void setBitOrder(BitOrder order);
void setDataMode(uint8_t uc_mode);
// void setClockDivider(uint8_t uc_div);
void setClockDivider(uint8_t div);
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

// // _p_sercom->setBaudrateSPI(div);
// // }
// }
void SPIClass::setClockDivider(uint8_t div)
Copy link
Contributor

Choose a reason for hiding this comment

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

Got these changes

oclyke added a commit that referenced this pull request Aug 5, 2019
@oclyke
Copy link
Contributor

oclyke commented Aug 5, 2019

I just took the meat of these changes and committed them with 74fb873. (Did this because there were many changes to master since this PR and I also did not want the bitOrder percolating up into the IOMaster code.) Closing. Thanks!

@oclyke oclyke closed this Aug 5, 2019
@oclyke oclyke deleted the spiClockDiv2 branch August 5, 2019 18:37
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