telemetry firmware first pass #80

aramachandran7 · 2021-04-06T14:36:14Z

CAN mailbox address switching State machine not included

jack-greenberg

Great start Adi. Some comments for you to address. Mostly to do with memory, and some clean up. I'll take another look with an eye closer on function after you fix this stuff.

jack-greenberg · 2021-04-12T20:06:11Z

telemetry/firmware/queue.h

+    }                                                                                   \
+}
+
+#ifdef DOXYGEN


Don't need any of this stuff. This is all for Doxygen, which we don't use

jack-greenberg · 2021-04-12T20:07:37Z

telemetry/firmware/telemetry.c

+        0xc0
+        0x10
+
+        do i create a custom mask that fits my needs? 


The range of IDs goes from 0xB to 0x63, so we can just have a mask that listens to any message in that range. So 0x7F should work as a mask.

Actually on second thought, we should just implement a MOb for 0xB, 0xC, 0xE, and 0x10 with the mask 0x1F, and then have a MOb for 0xc0 that only listens for that, and a MOb for 0x40-0x64.

I got the mask part wrong.

In a mask, if a bit is 0, that bit is ignored, and if the bit is 1, the bit must patch. So if you want to accept messages from 0x40 to 0x64, your mask will be 0x7C0.

But it will also accept messages up to 0x7F, so you'll need to do some additional checking

jack-greenberg · 2021-04-12T20:08:19Z

telemetry/firmware/telemetry.c

+#include <avr/interrupt.h>
+#include <util/delay.h>
+#include "can_api.h"
+#include "spi.h"


Not needed, need UART.

jack-greenberg · 2021-04-12T20:08:23Z

telemetry/firmware/telemetry.c

+#include <string.h>
+#include <avr/io.h>
+#include <avr/interrupt.h>
+#include <util/delay.h>


Not needed.

jack-greenberg · 2021-04-12T20:10:44Z

telemetry/firmware/telemetry.c

+volatile uint16_t shutdown_error_flag = 0x00; // system failure error code flag - default 0x00. 
+
+// CAN DATA to store & send out over UART 
+uint8_t accumulator_voltages[192] = {0}; // 192 wholeass bytes wtf


Watch your language

jack-greenberg · 2021-04-12T20:17:30Z

telemetry/firmware/telemetry.c

+		//Setup to Receive Again for future
+		CANSTMOB = 0x00;
+		CAN_wait_on_receive(BRAKE_LIGHT_MBOX, CAN_ID_BRAKE_LIGHT, CAN_LEN_BRAKE_LIGHT, CAN_MSK_SINGLE);
+	}


I know I said not to use malloc above, but just for the sake of completeness, you must free dynamically allocated memory at the end of usage. Call free(msg) (but actually just don't use malloc).

jack-greenberg · 2021-04-12T20:18:02Z

telemetry/firmware/telemetry.c

+
+
+            // we can free the data pointed to by temporary msg, as all bytes have been assigned & copied into the UART queue
+            free(temporary_msg); 


Don't need to free statically allocated memory.

jack-greenberg · 2021-04-12T20:20:30Z

telemetry/firmware/telemetry.c

+            handle_UART_recieved();
+        }
+    }
+}


Add newline.

jack-greenberg · 2021-04-12T20:21:08Z

telemetry/firmware/telemetry.c

+    uint8_t ID; 
+    short data_length; 
+    uint8_t data[8]; 
+} CAN_MSG; 


Might also want to have uint8_t mask as a part of this struct.

jack-greenberg · 2021-04-12T20:22:27Z

telemetry/firmware/telemetry.c

+/*----- Macro Definitions -----*/
+
+// general 
+#define FCLK                            160000000


Unused. Delete

jack-greenberg · 2021-04-12T20:44:38Z

telemetry/firmware/telemetry.c

+        CAN_MSG * msg = malloc(sizeof(CAN_MSG));  
+        (*msg) = { custom_ID, 8, can_recv_msg}; // TODO: To confirm, this copies over can_recv_mesg during asisgnment?
+
+        if (queue_CAN_push(&CAN_queue, msg) == 0) {


And then here, if msg has type CAN_MSG instead of CAN_MSG*, you would just use &msg to get the address of the msg struct.

…nts, simplified overall architecture to single queue

telemetry/firmware/queue.h

jack-greenberg · 2021-04-22T00:28:07Z

telemetry/firmware/queue.h

-\param ctx a pointer to arbitrary context that gets passed as the second argument to \c fun for each item in the queue
-*/
-#endif
+// #ifdef DOXYGEN


Don't comment it out, just delete it all.

jack-greenberg · 2021-04-22T00:29:04Z

telemetry/firmware/telemetry.c

-#define BRAKE_LIGHT_MBOX                   0
-#define BMS_CORE_MBOX                      1
-#define AIR_CONTROL_CRITICAL_MBOX          2
+#define THROTTLE_CMD_MBOX                  0


For clarity's sake, let's call these MOBs instead of MBOXes.

jack-greenberg · 2021-04-22T00:38:30Z

telemetry/firmware/telemetry.c

+
+ISR(CAN_INT_vect){ 
+
+    CANPAGE = (THROTTLE_BRD_MBOX<<MOBNB0); 


Datasheet 20.11.6/7, consider using the CANIE and CANSIT registers to tell which MOb triggered the interrupt.

jack-greenberg · 2021-04-22T00:40:15Z

telemetry/firmware/telemetry.c

+
+
+
+    // super loop 


Comment needed?

jack-greenberg · 2021-04-22T00:41:23Z

telemetry/firmware/telemetry.c

+        if (bit_is_set(gFlag, UART_SEND_READY)){
+            send_UART(); 
+        }
+        if (bit_is_set(gFlag, UART_RECIEVED)){


Unless I missed it, you don't have an interrupt on UART_RX_vect for receiving UART messages.

jack-greenberg · 2021-04-22T00:43:06Z

telemetry/firmware/telemetry.c

+            handle_UART_recieved();
+        }
+        if (shutdown_error_flag){
+            // TODO: need a clean way to handle this HIGH PRIORITY message & send immediately, without enqueueing? 


Not sure if we do. If we think about the use case, it would be best to see what the CAN messages right before shutdown are, and if we bypass the queue, we lose ordering. I think we should still use the queue. Shutdown shouldn't stop this board from working, so we'll continue transmitting telemetry data after shutdown.

awenstrup · 2021-07-04T15:53:01Z

telemetry/firmware/telemetry.c

+#define CELL_VOLT_TEMP_MOB                5
+
+#define CAN_MSK_6                           0b11000000 // custom mask for cell temps & voltages
+// handle another 15 mailboxes lol 


haha. fine for a first pass, but given that we may want to determine which messages are sent through telemetry dynamically in the future, we probably shouldn't be hard-coding these MOBs; we should be able to re-configure them without re-flashing code.

awenstrup · 2021-07-04T15:55:08Z

telemetry/firmware/telemetry.c

+// UART 
+
+// values within LINCR register
+#define ENABLE_LIN_AND_UART             LENA // can i leave this as LENA or does it have to be bit 3 (page 190)


I like this approach as opposed to hard-coding register values later in code like we usually do! You name the macros "enable XXX", but presumably only 1 bit in the register is actually doing the enabling; should we define those bits as macros as well?

awenstrup · 2021-07-04T15:57:11Z

telemetry/firmware/telemetry.c

+
+
+QUEUE(UART, uint8_t, UART_QUEUE_SIZE); 
+voltatile struct queue_UART UART_queue; // creating a UART_queue of type queue_UART 


probably don't need the comment here. I can tell you're creating a UART_queue of type queue_UART. If you want to include a comment, tell me what we're gonna put in this UART queue and where the data is going.

awenstrup · 2021-07-04T15:59:58Z

telemetry/firmware/telemetry.c

+/*----- Interrupt(s) -----*/
+
+
+ISR(CAN_INT_vect){ 


this function is huge, can we break it up? Here's roughly what I have in mind:

if bit_is_set(CANSIT2, MSG_XXX) { read_msg_xxx(); } void read_msg_xxx() { ... }

awenstrup · 2021-07-04T16:00:52Z

telemetry/firmware/telemetry.c

+}
+
+ISR(USART_TX_vect){
+    gFlag |= _BV(UART_SEND_READY); // set flag and get back to it 


awenstrup · 2021-07-04T16:06:19Z

telemetry/firmware/telemetry.c

+/*----- Functions -----*/
+
+
+void initUART(void){


looks like you're jumping around between using underscores and not using them. I don't have a strong preference either way (I think we usually use camelCase for C, no underscores). just please don't use both in the same file.

telemetry firmware first pass

41e0a13

jack-greenberg requested changes Apr 12, 2021

View reviewed changes

jack-greenberg reviewed Apr 12, 2021

View reviewed changes

completed 2nd pass at telemetry.c - added CAN support, fixed PR comme…

29d336f

…nts, simplified overall architecture to single queue

jack-greenberg requested changes Apr 22, 2021

View reviewed changes

relatively small best practice changes based on PR comments

91b391a

awenstrup reviewed Jul 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

telemetry firmware first pass #80

telemetry firmware first pass #80

aramachandran7 commented Apr 6, 2021

jack-greenberg left a comment

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

aramachandran7 Apr 13, 2021

aramachandran7 Apr 13, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 12, 2021

jack-greenberg Apr 22, 2021

jack-greenberg Apr 22, 2021

jack-greenberg Apr 22, 2021

jack-greenberg Apr 22, 2021

jack-greenberg Apr 22, 2021

jack-greenberg Apr 22, 2021

awenstrup Jul 4, 2021

awenstrup Jul 4, 2021

awenstrup Jul 4, 2021

awenstrup Jul 4, 2021

awenstrup Jul 4, 2021

awenstrup Jul 4, 2021



		// we can free the data pointed to by temporary msg, as all bytes have been assigned & copied into the UART queue
		free(temporary_msg);



		QUEUE(UART, uint8_t, UART_QUEUE_SIZE);
		voltatile struct queue_UART UART_queue; // creating a UART_queue of type queue_UART

		/----- Interrupt(s) -----/


		ISR(CAN_INT_vect){

telemetry firmware first pass #80

Are you sure you want to change the base?

telemetry firmware first pass #80

Conversation

aramachandran7 commented Apr 6, 2021

jack-greenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment