Created a UART API definition by RacingTornado · Pull Request #17 · mithro/fx2lib

RacingTornado · 2016-06-24T03:32:36Z

This defines a robust and flexible UART API which allows the example code to access the UART functionality in a standard manner irrespective of how the device is actually implementing this functionality.

mithro · 2016-06-24T04:06:40Z

include/softuart.h

@@ -0,0 +1,94 @@
+/** \file softuart.h


I recommend we go with include/uart/api.h

mithro · 2016-06-24T04:13:55Z

include/softuart.h

+ * This function uart0_receive is basically empty for fast_uart. However, in case of  
+ * timer based UART. It reads data from the queue, and returns a single character which 
+ * can then be used by the calling program.
+ *


See my comment about blocking verse non-blocking above. I think we probably need some type of function to check if there is a byte to receive.

mithro · 2016-06-24T05:14:21Z

include/uart/api.h

+ * \brief initalizes UART.
+ * This function uartX_init accepts the baud_rate and the mask.Returns 0 if successful.
+ * The mask indicates where the UART_TX pin must be attached. The rx_mask performs a 
+ * similar function. If FAST_UART is used, only transmit is allowed.


You are still talking about implementations here.

mithro · 2016-06-24T05:35:45Z

include/uart/api.h

+
+/**
+ * \brief initalizes UART.
+ * This function accepts mask for rx and tx.Returns 0 if successful.


Always put a space after a full stop.

mithro · 2016-06-24T05:41:16Z

Getting pretty close, I'm wondering if we should have shorter names than transmit and receive. Maybe rx and tx?

mithro · 2016-06-25T05:51:13Z

include/uart/api.h

+
+/**
+ * \brief Returns how many more bytes can be loaded
+ * into the buffer


Why does this comment wrap? You should be wrapping at 80 characters (or maybe 75).

mithro · 2016-06-25T05:52:03Z

You are still not putting full stops at the end of all your sentences in the documentation.

RacingTornado · 2016-06-25T06:20:36Z

You are still not putting full stops at the end of all your sentences in the documentation.

Fixed

mithro · 2016-06-27T04:57:55Z

include/uart/api.h

+ **/
+BOOL uartX_init(enum uart_baud rate, ...);
+
+/**


This documentation of this enum still hasn't been fixed.

Not really sure what kind of a doc you want. I just added 2 more line saying less than 0 not valid, greater than 0 valid. Is there something else you want me to add?

Take a look at other examples in the code. Remember consistency is important!

From fx2int.h

/** * \brief Interrupt numbers for standard FX2 interrupts. **/ typedef enum { IE0_ISR=0, ///< External interrupt 0 TF0_ISR, ///< Timer 0 interrupt IE1_ISR, ///< External interrupt 1 TF1_ISR, ///< Timer 1 interrupt TI_0_ISR, ///< Serial port 0 transmit or receive interrupt TF2_ISR, ///< Timer 2 interrupt RESUME_ISR, ///< Resume interrupt TI_1_ISR, ///< Serial port 1 transmit or receive interrupt USBINT_ISR, ///< USB Interrupt. An interrupt handler for this should only be used if not using auto vectored interrupts with INT2 I2CINT_ISR, ///< I2C Bus interrupt IE4_ISR, ///< External interrupt 4. An interrupt handler for this should only be used if not using auto vectored interrupts with INT4 IE5_ISR, ///< External interrupt 5 IE6_ISR, ///< External interrupt 6 // Better names for the USART interrupts USART0_ISR = TI_0_ISR, ///< Better name for USART0 interrupt USART1_ISR = TI_1_ISR, ///< Better name for USART1 interrupt } FX2_ISR;

From fx2macros.h

/** * \brief Used for getting and setting the CPU clock speed. **/ typedef enum { CLK_12M =0, CLK_24M, CLK_48M} CLK_SPD;

Everything else has a \brief for the short description. You haven't explained what the BAUD_ANY and BAUD_FASTEST should do.

This should probably be a typedef too.

I generally prefer not to use typedef. I prefer doing enum name. This is part of the Linux coding guidelines given https://www.kernel.org/doc/Documentation/CodingStyle , Chapter 5.

Please use the typedefs. This isn't the Linux kernel -- please use style consistent with the rest of the code.

mithro · 2016-06-27T04:59:30Z

Please fix the pull request description.

mithro · 2016-07-03T07:12:18Z

If you fix the enum documentation, this can be merged.

mithro · 2016-07-04T08:49:36Z

include/uart/api.h

+ * Enum values greater than 0 are supported.
+**/
+enum uart_baud {
+    BAUD_FASTEST = -2,	///< The fastest BAUD available on the system(Currently on 48Mhz,it is 1.71Mbps)


See commenting is important -- we had different understanding of this value. I think it should be "The fastest baud rate a UART supports".

Again, you shouldn't be referencing real implementations in the API document.

mithro · 2016-07-05T12:00:42Z

examples/lights/lights.c


 #include <lights.h>
 #include <delay.h>
+#include <uart/api.h>


mithro · 2016-07-11T04:41:21Z

include/uart/api.h

+ * \brief enum Standard available baud rates.
+ * BAUD_FASTEST will give you the fastest baud rate the UART can operate at. WARNING: 
+ * This is allowed to be a non-standard baud rate.
+ * Enum values greater than 0 are supported depending on the UART in use.To check


Space after full stop.

…access UART irrespective of the manner in which it is implemented. Updated

RacingTornado · 2016-07-20T06:20:17Z

I have changed things to the best of my knowledge.

RacingTornado force-pushed the merge_uart_api branch from 916a82a to 491be57 Compare June 24, 2016 03:54

mithro reviewed Jun 24, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch from 491be57 to e6d4371 Compare June 24, 2016 04:13

mithro reviewed Jun 24, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch 2 times, most recently from 572d7cd to 9685276 Compare June 24, 2016 04:46

mithro reviewed Jun 24, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch 3 times, most recently from 3083e9c to 075d0ab Compare June 24, 2016 05:29

mithro reviewed Jun 24, 2016
View reviewed changes

mithro reviewed Jun 25, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch from 7301453 to dbba734 Compare June 25, 2016 06:20

mithro changed the title ~~Created a UART API definition for allowing proper abstraction~~ Created a UART API definition Jun 27, 2016

mithro reviewed Jun 27, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch from dbba734 to 3342242 Compare June 27, 2016 05:10

RacingTornado force-pushed the merge_uart_api branch from 3342242 to 4be57ba Compare July 4, 2016 07:45

mithro reviewed Jul 4, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch from 4be57ba to e5de956 Compare July 5, 2016 05:57

mithro reviewed Jul 5, 2016
View reviewed changes

examples/lights/lights.c Outdated

#include <lights.h>

#include <delay.h>

#include <uart/api.h>

Copy link

Owner

mithro Jul 5, 2016

Choose a reason for hiding this comment

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

Um?

RacingTornado force-pushed the merge_uart_api branch 2 times, most recently from 84e7117 to dadddb1 Compare July 5, 2016 13:19

mithro reviewed Jul 11, 2016
View reviewed changes

RacingTornado force-pushed the merge_uart_api branch 3 times, most recently from d4322d3 to b95e144 Compare July 20, 2016 06:13

Created a UART API definition in softuart.h to allow the firmware to …

a3c3e01

…access UART irrespective of the manner in which it is implemented. Updated

RacingTornado force-pushed the merge_uart_api branch from b95e144 to a3c3e01 Compare July 20, 2016 06:16

Conversation

RacingTornado commented Jun 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mithro commented Jun 24, 2016

Uh oh!

mithro Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mithro commented Jun 25, 2016

Uh oh!

RacingTornado commented Jun 25, 2016

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!

mithro commented Jun 27, 2016

Uh oh!

mithro commented Jul 3, 2016

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!

RacingTornado commented Jul 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RacingTornado commented Jun 24, 2016 •

edited

Loading

mithro Jun 25, 2016 •

edited

Loading