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

Doc: spi documentation fixes #8516

Merged
merged 7 commits into from Oct 27, 2018

Conversation

Projects
None yet
5 participants
@paul-szczepanek-arm
Member

paul-szczepanek-arm commented Oct 23, 2018

Description

Fixed missing and incorrect descriptions, style and grammar.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[x] Docs update
[ ] Test update
[ ] Breaking change
*
* @return Operation success.
* @retval 0 A transfer was started or added to the queue.
* @retval -1 Transfer can't be added because queue is full.

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Oct 23, 2018

Member

there is actually a bug in the implementation, it doesn't forward the error code, will file an issue

@cmonr cmonr requested a review from melwee01 Oct 23, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 23, 2018

@paul-szczepanek-arm Please take another look at your #if/#endif declarations.

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Oct 24, 2018

Thanks, apologies, never a good idea to commit at the end of the day.

@melwee01

Do we need \brief before /** sections?

@@ -36,39 +36,44 @@
namespace mbed {
/** \addtogroup drivers */
/** A SPI Master, used for communicating with SPI slave devices
/** A SPI Master, used for communicating with SPI slave devices.

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

An SPI

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Oct 24, 2018

Member

It's pronounced "spy". I think 'an' vs 'a' is decided by pronunciation. So it's "an honour" and I can't think of a counter example.

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

Ah. Right, in that case, carry on.

I'm quite pleased it IS a 'spymaster' after all!

*
* mosi or miso can be specified as NC if not used
* @note Either mosi or miso can be specified as NC if not used.

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

Not sure what device-side standards are, but we usually replace passive constructions like this with 'you can'.

*/
SPI(PinName mosi, PinName miso, PinName sclk, PinName ssel = NC);
virtual ~SPI();
/** Configure the data transmission format
/** Configure the data transmission format.

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

Do these need \brief commands/tags/whatever they're called?

This comment has been minimized.

@paul-szczepanek-arm

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

👍

int queue_transfer(const void *tx_buffer, int tx_length, void *rx_buffer, int rx_length, unsigned char bit_width, const event_callback_t &callback, int event);
/** Configures a callback, spi peripheral and initiate a new transfer
/** Configure a callback, spi peripheral and initiate a new transfer.

This comment has been minimized.

@melwee01

melwee01 Oct 24, 2018

Contributor

SPI is capitalized in some places and not others.

paul-szczepanek-arm added some commits Oct 24, 2018

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Oct 24, 2018

can you re-review please @melinda?

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Oct 24, 2018

looks like that littlefs test has the DOXYGEN_ONLY macro defined?

@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 24, 2018

Comments addressed. Please re-review.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 25, 2018

@geky Question about the Travis CI check that's failing in this PR.

Is that snippet doing a similar thing where it parses and runs code comment blocks? And does that mean this PR is failing because the comment(s) fail to compile? 😄

*
* @returns
* Response from the SPI slave
* @return Response from the SPI slave

This comment has been minimized.

@melwee01

melwee01 Oct 25, 2018

Contributor

Missing period.

@@ -179,75 +186,85 @@ class SPI : private NonCopyable<SPI> {
return 0;
}
/** Abort the on-going SPI transfer, and continue with transfer's in the queue if any.
/** Abort the on-going SPI transfer, and continue with transfers in the queue if any.

This comment has been minimized.

@melwee01

melwee01 Oct 25, 2018

Contributor

... in the queue, if any.

int queue_transfer(const void *tx_buffer, int tx_length, void *rx_buffer, int rx_length, unsigned char bit_width, const event_callback_t &callback, int event);
/** Configures a callback, spi peripheral and initiate a new transfer
/** Configure a callback, SPI peripheral and initiate a new transfer.

This comment has been minimized.

@melwee01

melwee01 Oct 25, 2018

Contributor

... callback, SPI peripheral, and initiate...

@melwee01

This comment has been minimized.

Contributor

melwee01 commented Oct 25, 2018

Re-review done.

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Oct 25, 2018

Applied your requested changes

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

@paul-szczepanek-arm Can you review travis failure? it can't compile SPI.cpp file

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 25, 2018

@0xc0170 Waiting to hear back from @geky about the script. I suspect the Travis command that is failing is attempting to compile commented code.

void start_transfer(const void *tx_buffer, int tx_length, void *rx_buffer, int rx_length, unsigned char bit_width, const event_callback_t &callback, int event);
#if !defined(DOXYGEN_ONLY)

This comment has been minimized.

@geky

geky Oct 25, 2018

Member

It looks like this is causing the travis failure. @0xc0170 @cmonr does anyone know if DOXYGEN_ONLY is set for all travis jobs?

@geky

This comment has been minimized.

Member

geky commented Oct 25, 2018

Found the issue, the ifdefs were interleaved and would fail depending on the configuration of DEVICE_SPI_ASYNCH, @paul-szczepanek-arm let me know if you're happy with this.

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Oct 26, 2018

good catch, thank you!

@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 26, 2018

@cmonr

cmonr approved these changes Oct 26, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 26, 2018

Note: This PR is now a part of a rollup PR (#8552).

No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged.

If any more commits are made to this PR, this will have to go through CI on it's own.

@cmonr cmonr referenced this pull request Oct 26, 2018

Merged

Rollup PR: UK Docathon pt2 #8552

@cmonr cmonr merged commit fa09fff into ARMmbed:master Oct 27, 2018

11 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 536 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9279 cycles (-727 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment