Sinapse-Energia / MIFARE-US

2 stars 0 forks source link

[Main] To define & develop the main structure #8

Open ralcaide opened 7 years ago

ralcaide commented 7 years ago

Main (Loop) that implements the behavior of the project

soporteHW commented 7 years ago

Attached a draft copy for MAIN loop implementation.

main_mifare.txt

ralcaide commented 7 years ago

Reviewed the first version of the main. Attached the file with my comments. The main should be a real code and I miss some information. @soporteHW , please take into account my comments and upload the next main.c to the the repository. It is easy to review the code when it is github

main_mifare_RAE_comments.txt

soporteHW commented 7 years ago

@ralcaide the previous file is a eraser copy, so it's uncompleted. My intention was to write the main structure (more or less in pseudocode format) in order to having a global vision. Obviously, calls to the functions are incomplete.

I will take your comments into account for the next review.

francisjjp commented 7 years ago

Regarding main.c with rafaide comments. I put also my comments inside this code, or pseudocode.

int main(void) {

FJP Comment : Above this point, functions are for initialitations peripherals.

/ USER CODE BEGIN 2 /

/NTP Synchronization: Read time from server and set into uC RTC/ TCP_IP_Connect(); // RAE : Connect to where? URL and Port are parameters or they are hardcoded?

FJP: @soporteHW . Parameters must be included here, TCP_IP_Connect must return 1 if all is OK.

TCP_IP_Get_Data() // RAE: What data? This call should be to Get_NTP() that inside call the Generic Get_Data to get the ntp time, isn't?

FJP: @soporteHW . It is needed to create Get_NTP() functions that will use TCP_IP_Get_Data(), this is a lower layer and main function to get data from one TCP connection. The call always must have parameter. Try no to use so many pseudocode, if not, the exercise of writing main.c becomes bizarre.

MIC_Set_RTC(); // RAE: What Time? This time should be a paramater, isn't?

FJP: @soporteHW, please write function with exact parameters. If you write them now, it will help you with understanding of main.c

/Update Context: Read Flash Memory and update context if necessary / MIC_Flash_Memory_Read(); // RAE: This function gets data from the NVM Flash and these data should be saved in the RAM. You are calling directly the generic Read to do everything??. Where should read? positions?

*FJP: @ralcaide . I understand that this function will retrieve all data from flash and it will save them directly into one RAM global structure. i.e: RAM_global_structure. Then the function call should be : `MIC_Flash_Memory_Read((uint8_t)&RAM_global_structure, sizeof(RAM_global_structure))`**

MIC_Flash_Memory_Write(); // RAE: What is necessary to write? where?

FJP: @soporteHW . I don't understand the MIC_Flash_Memory_Write() function here. Only, you must write entire data into Flash, when some "trigger" has changed something into context. i.e: Maybe, when we have get some data from RFID, etc. Only, it is needed the call to this function when some data context has changed.

/Initialize, Set LCD Display config and show status message/ LCD_Init(); LCD_Write_mifare_info(); //RAE: OK, this function sets the default message into the LCD, isn't?

FJP: @soporteHW , please, put always parameters

/ USER CODE END 2 /

/ Infinite loop / / USER CODE BEGIN WHILE / while (1) { / USER CODE END WHILE /

  /*Waiting for UART Interrupt*/

   if (flag_interrupt == 1) //RAE: Declaration / initialization of this var?

FJP: @soporteHW , If you write pseudcode, at least, define the meaning of variables. If you use code, declare the variable and with some comment explain its meaning. I suspect that is variable that is updated with MIFARE interrupt is detected. @soporteHW Are you tied some RFID line as hardware interrupt. If not, please explain when this variable is set to 1.

  {
      /*Reading MIFARE card: block 3, sector 16, key FFFFFFFFFFFF*/
      RFID_Read_Memory_Block(); // RAE: This function should receive the parameters of what / where read. The result should be saved into the RFID_OK

FJP: @soporteHW , the memory position must be the parameter of above function. Output is RFID_OK or RFID_NOK, is it?. You must have one parameter with read data too. I.E. RFID_read_data

       if(RFID_OK) // RAE: Declaration / initialization of this var?
      {
          /*Beep & Update LCD*/
          Blink_LED_Status(Reading);
          Buzzer_Control(); // RAE: Without parameter?

FJP: @soporteHW. 'Reading' parameters is when all goes right, is it?. Then you should put same parameter into Buzzer_Control().

          /*Create the HTTP message, connect to HTTP server and send HTTP msg*/

          // RAE: Where is the message created??. Connect to where? 
          TCP_IP_Connect();
          TCP_Send_Data();

FJP: @soporteHW. As before, you must include parameters where right ID is included int HTTP message.

          /*Wait for server response until timeout*/
          if(response_OK) // RAE: Where is received the response??
          {

FJP: @soporteHW , response_OK must be explained pseudocode) or declared (code) /Beep & Update LCD/ Blink_LED_Status(Reading); Buzzer_Control(); // RAE: Without parameter?

FJP: @soporteHW. 'Reading' parameters is when all goes right, is it?. Then you should put same parameter into Buzzer_Control(). } else (response_NOK) // RAE: Where is received the response?? { /Beep & Update LCD/ Blink_LED_Status(Reading); Buzzer_Control(); // RAE: Without parameter?

FJP: @soporteHW. This part of code is for management of device when something goes wrong with HTTP response. 'Reading' parameters must be different from previous call when all is right. Either update Reading variable or change it for constant as.. FAILED. Then you should put same parameter into Buzzer_Control(). I.E. Buzzer_Control(FAILED). } // RAE: I'm missing the update of the LCD. It should be updated after the HTTP comms, isn't??

FJP: @ralcaide , Yes, LCD should be updated as Blink_LED_Status and Buzzer_Control. In fact, maybe is useful to use one function that updates three devices at same time. LCD, LED, Buzzer.

      }
      else (RFID_NOK) // RAE: Where is received the response??
      {
        /*Beep & Update LCD*/
        Blink_LED_Status(Reading);
        Buzzer_Control(); // RAE: Without parameter?

FJP: @soporteHW. This part of code is for management of device when something goes wrong with RFID response. 'Reading' parameters must be different from previous call when all is right. Either update Reading variable or change it for constant as.. FAILED. Then you should put same parameter into Buzzer_Control(). I.E. Buzzer_Control(FAILED). Even if you want to put something different from HTTP fail, you can put FAILED_HTTP and FAILED_RFID constants.

      }
  }

  else (flag_interrupt != 1)
  {
    /*Update LCD Display*/
    LCD_Write_mifare_info();

FJP: @soporteHW. I don't understand this part

  }   

/ USER CODE BEGIN 3 /

} / USER CODE END 3 /

}

soporteHW commented 7 years ago

The main structure is almost developed. Now, it's necessary to remove unusable code and introduce self-help comments to leave a readable user code.

Also, all SB functions must be commented.