I2C slave & clock calibration by AugustinBoisvert · Pull Request #1 · robotter/avarix

AugustinBoisvert · 2017-07-22T18:32:30Z

No description provided.

benoitryder

Main comment: fix the many coding style inconsistencies. I only checked rapidly for other things but will check in details after style has been fixed.

This review is a bit messy as I started to review commits one by one, then changes as a whole (since the 2d I2C commit updates previous changes).

benoitryder · 2017-07-22T20:46:58Z

 #define CLOCK_PER2_FREQ  CLOCK_CPU_FREQ
 #define CLOCK_PER4_FREQ  CLOCK_CPU_FREQ

+// uncomment the following line to activate automatic run-time calibration of internal clock


Variable should be documented using Doxygen syntax.
Something like this (need to check if Doxygen autodoc #undef too):

/** @brief Active automatic run-time calibration of internal clock * ... */ #undef CLOCK_AUTOMATIC_RUN_TIME_CALIBRATION

benoitryder · 2017-07-22T20:48:15Z


+#ifdef CLOCK_AUTOMATIC_RUN_TIME_CALIBRATION
+# if CLOCK_SOURCE == CLOCK_SOURCE_RC2M
+  DFLLRC2M.CTRL = DFLL_ENABLE_bm;


This line should be indented with 3 spaces (same below).

corrected in ef2ed19

benoitryder · 2017-07-22T20:52:44Z

 #elif (defined I2CC_SLAVE)
-extern i2cs_t *const i2cC;
+# define X_(p,s)  p ## C ## s
+#include "slavex.inc.h"


Missing indent (one space), same below.

benoitryder · 2017-07-22T20:53:43Z

+//@{
+/**
+ * @file
+ * @brief I2C definitions


Inaccurate comment (copied from i2c.h actually).

benoitryder · 2017-07-22T20:54:55Z

+#include "i2c_config.h"
+
+/* Transaction status defines.*/
+#define I2CS_STATUS_READY                0


Coding style: don't align like this, use two spaces after the definition name.

benoitryder · 2017-07-22T21:16:24Z

+ */
+void i2cX(_receive_handler(void)) {
+  /* Enable stop interrupt. */
+  uint8_t currentCtrlA = TWIX.SLAVE.CTRLA;


Don't use camelCase variable names.

benoitryder · 2017-07-22T21:16:53Z

+    uint8_t data = TWIX.SLAVE.DATA;
+    i2csX->received_data[i2csX->bytes_received] = data;
+
+    /* Process data. */


Useless comment.

benoitryder · 2017-07-22T21:22:26Z

 #include <avr/io.h>
 #include <avarix/intlvl.h>
 #include <stdint.h>
+#include <stddef.h>


What requires this header?

The use of NULL keyword in slavex.inc.c file.
It has been moved in slavex.inc.c in ef2ed19

benoitryder · 2017-07-22T21:26:51Z

+
+/* Transaction result enumeration */
+typedef enum {
+  I2CS_RESULT_UNKNOWN            = 0,


Don't align commas (and equal).

Also, why using (0x01<<0) and not simply 1?

benoitryder · 2017-07-22T21:27:59Z

+  I2CS_RESULT_ABORTED           
+} i2c_result_t;
+
+/*! \brief i2c slave driver struct.


For consistency:

use /** @brief

Use I2C (not i2c) in comments (unless speaking of the module or a symbol name).

And as above, don't try align comments.

benoitryder

Second pass. Will read actual I2C code more carefully at a better time (not so late).

benoitryder · 2017-07-25T00:40:02Z

 #define CLOCK_PER2_FREQ  CLOCK_CPU_FREQ
 #define CLOCK_PER4_FREQ  CLOCK_CPU_FREQ

+/** @brief Activates automatic run-time calibration of internal clock


@brief should be separated from detailed information by starting a new paragraphe (empty line).
Also, details should be properly capitalized and punctuated, and only separated by one space from the comment block's *.
Other occurrences below.

benoitryder · 2017-07-25T00:41:30Z

+//@{
+/**
+ * @file
+ * @brief I2Cs definitions


That's a lazy change (what is the meaning of I2Cs?). I2C slave definitions would be better.

benoitryder · 2017-07-25T00:42:16Z

+#include <stdint.h>
+#include "i2c_config.h"
+
+// Transaction status defines


Use /// for 1-line documentation of elements in Doxygen.
This is equivalent to /** @brief ... */.
(Other occurrences below.)

benoitryder · 2017-07-25T00:42:35Z

+  I2CS_RESULT_TRANSMIT_COLLISION,
+  I2CS_RESULT_BUS_ERROR,
+  I2CS_RESULT_FAIL,
+  I2CS_RESULT_ABORTED


Please, add a trailing comma after the last element.

benoitryder · 2017-07-25T00:43:27Z

+  I2CS_RESULT_ABORTED
+} i2c_result_t;
+
+/* @brief I2C slave driver struct.


/** instead of /*
(Other occurrences below.)

benoitryder · 2017-07-25T00:47:46Z

+ */
+static void i2cX(_transmit_handler(void)) {
+  /* If NACK, slave transmit transaction finished. */
+  if ((i2cX().bytes_transmit > 0) && (TWIX.SLAVE.STATUS &


Remove superfluous parenthesis around first condition.

Remove extra space after if.

Other occurrences below.

benoitryder · 2017-07-25T00:48:52Z

+    /* If buffer overflow. */
+    else {
+      TWIX.SLAVE.CTRLB = TWI_SLAVE_CMD_COMPTRANS_gc;
+      i2cX(_transaction_finished(I2CS_RESULT_BUFFER_OVERFLOW));


Should be i2cX(_transaction_finished)(I2CS_RESULT_BUFFER_OVERFLOW);

Similar occurrences below.

benoitryder · 2017-07-25T00:51:26Z


-#error Slave mode not supported yet
+#define I2CX(s) X_(I2C,s)
+#define i2cX(s) X_(i2c,s)


You should define and use i2csX(s) X_(i2cs,s) instead, for method names and everything that is slave-specific.

benoitryder · 2017-07-25T00:53:06Z

+           (current_status & TWI_SLAVE_AP_bm)) {
+    i2cX(_address_match_handler());
+    /* Process data. */
+    if(i2cX().process_data != NULL)


Always enclose if/else/... bodies with {...}.
(Multiple occurences.)

benoitryder · 2017-07-25T00:54:31Z

+
+
+/* I2cX Register callback */
+void i2cX(s_register_callback)( void (*process_data_function)(void));


Extra space after (.

… data receive or transmit

benoitryder · 2018-05-29T17:22:19Z

 #define CLAMP(v,x,y) ((v) < (x) ? (x) : (v) > (y) ? (y) : (v))
-
+/// Get number of elements in an array
+#define LENGTHOF(a) (sizeof(a)/sizeof((a)[0]))


Version un peu meilleure :

#define LENGTHOF(a) ((sizeof(a) / sizeof((a)[0])) / (sizeof(a) % sizeof((a)[0]) == 0))

Ça permet de détecter à la compilation certains cas (pas tous malheureusement) où on passe un pointeur au lieu d'un tableau.
Sinon pour être 100% safe, y'a la méthode Russell, qui pique un peu quand même. ;)

benoitryder · 2018-05-29T17:23:15Z

- *
- * Only usable with 2MHz or 32Mhz internal clocks.
- * It will use the 32768kHz calibrated clock as the adjustement source.
+/** @brief Activates automatic run-time calibration of internal clock


J'avais déjà mergé cette partie. Mieux vaut garder la version actuelle (déjà utilisée).

benoitryder · 2018-05-29T17:25:32Z

  return i;
 }

+int8_t i2cs_send_async(i2cs_t *s, uint8_t *data, uint8_t n) {


benoitryder · 2018-05-29T17:29:59Z

+} i2cs_state_t;
+
+#ifndef I2CS_RECV_BUFFER_SIZE
+#define I2CS_RECV_BUFFER_SIZE 32


Coding style :

indentation du define

double espace avant la valeur
(idem plus bas)

benoitryder · 2018-05-29T17:31:39Z

+#define I2CS_SEND_BUFFER_SIZE 32
+#endif
+
+struct i2cs;


Forward declaration non nécessaire.

benoitryder · 2018-05-29T17:55:02Z

+          i2cs->sent_bytes = 0;
+
+          // ask user to provision send buffer
+          int rsz = i2cs->send_callback(i2cs->send_buffer,


La valeur de retour pourrait être un uint8_t.

benoitryder · 2018-05-29T17:56:46Z

+                                          sizeof(i2cs->send_buffer));
+          if(rsz > 0) {
+            // user got some data to send
+            i2cs->bytes_to_send = MIN(rsz,I2CS_SEND_BUFFER_SIZE);


Par souci de cohérence, n'utilises pas tantôt sizeof(i2cs->send_buffer), tantôt I2CS_SEND_BUFFER_SIZE.

Espace après la virgule.

Si bytes_to_send est trop grand, on devrait gérer ça comme une erreur plutôt que de passer ça sous silence, au moins l'utilisateur serait au courant.

benoitryder · 2018-05-29T18:00:22Z

+//@{
+/**
+ * @file
+ * @brief I2C definitions


I2C slave definitions

benoitryder · 2018-05-29T18:02:11Z

+
+  int sent_bytes;
+  int bytes_to_send;
+  uint8_t send_buffer[I2CS_SEND_BUFFER_SIZE];


Sauf erreur, on ne fait pas lire et écrire simultanément.
Les mêmes variables (notamment le buffer) peuvent être utilisées pour la lecture et l'écriture.

benoitryder · 2018-05-29T18:02:24Z

+
+  i2cs_state_t state;
+
+  int recvd_bytes;


Utiliser des uint8_t quand c'est possible.

benoitryder · 2018-06-01T19:17:16Z

+// Interrupt handler
+ISR(twiX(_TWIS_vect)) {
+
+  i2cs_t *i2cs = &i2cX();


Ok.
Côté coût il ne devrait pas y avoir de différence (et le compilo devrait être capable d'optimiser).
C'était juste une question de style.

benoitryder · 2018-06-01T19:18:13Z

+  uint8_t send_buffer[I2CS_SEND_BUFFER_SIZE];
+
+  i2cs_recv_callback_t recv_callback;
+


Tu peux retirer les lignes vides entre chaque définition de callback.

benoitryder · 2018-06-01T19:20:38Z

+
+  uint8_t sent_bytes;
+  uint8_t bytes_to_send;
+  uint8_t send_buffer[I2CS_SEND_BUFFER_SIZE];


Ton avis sur la possibilité de mutualiser les variables recv/send (notamment les buffers) ?
(Sachant que tu peux utiliser une union pour garder des noms pertinents au besoin.)

benoitryder · 2018-06-01T19:24:47Z

+  TWIX.SLAVE.ADDR = I2CX(_ADDRESS) << 1;
+}
+
+void i2cX(s_register_reset_callback)(i2cs_reset_callback_t f)


Une raison pour ne pas plutôt avoir une fonction qui prend en paramètre le i2cs_t* à modifier ?
Pour l'init on peut rester avec une fonction part I2C (vu que les init sont appellées d'un bloc par le module, de mémoire). Par contre pour les accesseurs ça rend la lib plus générique (pas besoin de connaître le nom de la fonction, on a juste à fournir le bon objet).

C'est comme ça que fonctionne le module UART notamment.

benoitryder · 2018-06-01T19:26:35Z

+}
+
+// Interrupt handler
+ISR(twiX(_TWIS_vect))


La routine d'interruption étant conséquente, ça serait intéressant d'ajouter une indirection vers une fonction générique qui prendrait un i2cs_t* en paramètre.

Là encore, ça suit ce qui est fait pour l'UART.

benoitryder requested changes Jul 22, 2017

View reviewed changes

AugustinBoisvert added a commit that referenced this pull request Jul 24, 2017

coding style corrections #1

ef2ed19

AugustinBoisvert force-pushed the dev branch from ef2ed19 to 1b9f0dd Compare July 24, 2017 12:13

AugustinBoisvert added a commit that referenced this pull request Jul 24, 2017

coding style corrections #1

1b9f0dd

benoitryder requested changes Jul 25, 2017

View reviewed changes

rmirault and others added 4 commits May 24, 2018 14:01

i2c: add i2c slave, first build ok

a5fdacd

i2c: I2cX data structure singleton changed and callback added for all…

256601a

… data receive or transmit

clock : add run-time autocalibration option

92712d1

coding style corrections #1

a12d18a

JDRobotter force-pushed the dev branch from 1b9f0dd to a12d18a Compare May 24, 2018 12:03

JD added 2 commits May 26, 2018 23:24

i2c slave reworked, more documentation needed

504bae6

LENGTHOF() added to avarix.h

2052d1f

benoitryder requested changes May 29, 2018

View reviewed changes

JDRobotter force-pushed the dev branch from f3855be to 2052d1f Compare May 30, 2018 17:37

JD added 2 commits June 1, 2018 13:23

i2cs: fixes from PR

32fb9f0

i2cs: more fixes from PR

258733f

benoitryder requested changes Jun 1, 2018

View reviewed changes



		/* I2cX Register callback */
		void i2cX(s_register_callback)( void (*process_data_function)(void));

		uint8_t send_buffer[I2CS_SEND_BUFFER_SIZE];

		i2cs_recv_callback_t recv_callback;

Uh oh!

Conversation

AugustinBoisvert commented Jul 22, 2017

Uh oh!

benoitryder 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!

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!

benoitryder 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!

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!

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!