Doc: spi documentation fixes #8516

paul-szczepanek-arm · 2018-10-23T16:28:03Z

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

paul-szczepanek-arm · 2018-10-23T16:32:57Z

drivers/SPI.h

+     *
+     * @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.


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

cmonr · 2018-10-23T18:30:25Z

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

paul-szczepanek-arm · 2018-10-24T07:50:45Z

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

melwee01

Do we need \brief before /** sections?

melwee01 · 2018-10-24T06:40:14Z

drivers/SPI.h

@@ -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.


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.

Ah. Right, in that case, carry on.

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

melwee01 · 2018-10-24T08:09:02Z

drivers/SPI.h

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


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

melwee01 · 2018-10-24T08:09:35Z

drivers/SPI.h

     */
    SPI(PinName mosi, PinName miso, PinName sclk, PinName ssel = NC);
    virtual ~SPI();

-    /** Configure the data transmission format
+    /** Configure the data transmission format.


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

No. @brief is implied.

melwee01 · 2018-10-24T08:13:21Z

drivers/SPI.h

    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.


SPI is capitalized in some places and not others.

paul-szczepanek-arm · 2018-10-24T09:12:29Z

can you re-review please @melinda?

paul-szczepanek-arm · 2018-10-24T10:44:37Z

looks like that littlefs test has the DOXYGEN_ONLY macro defined?

Comments addressed. Please re-review.

cmonr · 2018-10-25T00:44:11Z

@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? 😄

melwee01 · 2018-10-25T10:29:53Z

drivers/SPI.h

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


Missing period.

melwee01 · 2018-10-25T10:30:14Z

drivers/SPI.h

@@ -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.


... in the queue, if any.

melwee01 · 2018-10-25T10:30:54Z

drivers/SPI.h

    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.


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

melwee01 · 2018-10-25T10:31:59Z

Re-review done.

paul-szczepanek-arm · 2018-10-25T10:36:23Z

Applied your requested changes

0xc0170 · 2018-10-25T11:44:05Z

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

cmonr · 2018-10-25T13:21:02Z

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

geky · 2018-10-25T16:09:54Z

drivers/SPI.h

    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)


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

geky · 2018-10-25T16:35:15Z

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 · 2018-10-26T07:15:59Z

good catch, thank you!

cmonr · 2018-10-26T16:56:45Z

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.

spi doxygen fixes

482f2b6

paul-szczepanek-arm requested a review from AnotherButler October 23, 2018 16:28

imperative

263f067

paul-szczepanek-arm commented Oct 23, 2018

View reviewed changes

cmonr added needs: review rollup PR labels Oct 23, 2018

cmonr requested a review from melwee01 October 23, 2018 16:54

cmonr added needs: work and removed needs: review rollup PR labels Oct 23, 2018

fix syntax err from misplaced ifdef

68628c1

melwee01 previously requested changes Oct 24, 2018

View reviewed changes

paul-szczepanek-arm added 2 commits October 24, 2018 09:26

SPI capitalisation

d4b6fdc

active voice

efdcdd5

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

cmonr added rollup PR release-version: 5.10.3 labels Oct 25, 2018

melwee01 reviewed Oct 25, 2018

View reviewed changes

Update SPI.h

889a98e

cmonr added needs: work and removed needs: review rollup PR labels Oct 25, 2018

geky reviewed Oct 25, 2018

View reviewed changes

Fix interleaved ifdefs with DEVICE_SPI_ASYNCH + DOXYGEN_ONLY

fa09fff

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

cmonr approved these changes Oct 26, 2018

View reviewed changes

cmonr added needs: CI rollup PR and removed needs: review labels Oct 26, 2018

cmonr mentioned this pull request Oct 26, 2018

Rollup PR: UK Docathon pt2 #8552

Merged

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

cmonr removed needs: CI rollup PR labels Oct 27, 2018

		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)

Doc: spi documentation fixes #8516

Doc: spi documentation fixes #8516

Uh oh!

Conversation

paul-szczepanek-arm commented Oct 23, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Oct 23, 2018

Uh oh!

paul-szczepanek-arm commented Oct 24, 2018

Uh oh!

melwee01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm commented Oct 24, 2018

Uh oh!

paul-szczepanek-arm commented Oct 24, 2018

Uh oh!

cmonr commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melwee01 commented Oct 25, 2018

Uh oh!

paul-szczepanek-arm commented Oct 25, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

cmonr commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Oct 25, 2018

Uh oh!

paul-szczepanek-arm commented Oct 26, 2018

Uh oh!

cmonr commented Oct 26, 2018

Uh oh!

Uh oh!