Merge datastorage framework in to main #12

Closed
Athamantis wants to merge 0 commits from eeprom into main
Owner

closes #7

closes #7
Athamantis added 6 commits 2024-03-13 09:36:40 +00:00
sebgab requested review from sebgab 2024-03-13 09:41:30 +00:00
sebgab requested review from Inamr 2024-03-13 09:41:40 +00:00
Inamr approved these changes 2024-03-13 10:10:47 +00:00
Inamr left a comment
Owner

LGTM OwO

LGTM OwO
sebgab requested changes 2024-03-13 10:17:57 +00:00
sebgab left a comment
Owner

Needs a couple changes.

Needs a couple changes.
@ -0,0 +1,98 @@
#include "eeprom.h"
// The start address for the controller data
uint8_t EEMEM startAddressController = 0x00;
Owner

Can these be #defines in the header file?
They feel like they should be defines in the header file.

Can these be #defines in the header file? They feel like they should be defines in the header file.
Author
Owner

No, they can't. The EEMEM does not allow it do be as defined, and it does not like being in the header.

No, they can't. The EEMEM does not allow it do be as defined, and it does not like being in the header.
Athamantis marked this conversation as resolved
@ -0,0 +16,4 @@
while(1){
if (eeprom_is_ready()){
break;
printf("it is");
Owner

Can this printf be reached if it is behind break?
If it is dead code it should be removed.

Can this printf be reached if it is behind break? If it is dead code it should be removed.
Author
Owner

Fixed in 4a8b6137d1

Fixed in 4a8b6137d140fee8638b974935649dee05830b58
Athamantis marked this conversation as resolved
@ -0,0 +19,4 @@
printf("it is");
}else{
;
printf("its not");
Owner

Printing every cycle feels redundant. Remove?

Printing every cycle feels redundant. Remove?
Author
Owner

Fixed in 4a8b6137d1

Fixed in 4a8b6137d140fee8638b974935649dee05830b58
Athamantis marked this conversation as resolved
@ -0,0 +30,4 @@
// Checks if it has been written information at the address
// If true, the infromation is replaced with the intaken struct
// else the intaken struct is written at the address.
void WriteStructInEEPROM(config_t writeStruct){
Owner

Function names should use a standardized naming scheme.

I have used word_another() so far, what about you @Inamr ?

Function names should use a standardized naming scheme. I have used `word_another()` so far, what about you @Inamr ?
Author
Owner

ina used word_another(), I wil be changing the standarized naming sheme

ina used word_another(), I wil be changing the standarized naming sheme
Author
Owner

Fixed in 4a8b6137d1

Fixed in 4a8b6137d140fee8638b974935649dee05830b58
Athamantis marked this conversation as resolved
@ -0,0 +32,4 @@
// else the intaken struct is written at the address.
void WriteStructInEEPROM(config_t writeStruct){
uint8_t structSize;
structSize = sizeof(writeStruct);
Owner

This should be assigned on the previous line.

This should be assigned on the previous line.
Athamantis marked this conversation as resolved
@ -0,0 +40,4 @@
}
// Reads the memory block at the address 0x00
Owner

This comment should not say 0x00, it should name the variable that it actually uses. Even if that is 0x00 at this moment that might change, which would make the comment invalid.

This comment should not say 0x00, it should name the variable that it actually uses. Even if that is 0x00 at this moment that might change, which would make the comment invalid.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -0,0 +42,4 @@
// Reads the memory block at the address 0x00
// returns a struct in form of config_t
config_t ReadStructInEEPROM(){
Owner

Read/WriteStructInEEPROM() functions should probably be called Read/WriteStructFromEEPROM() for proper english.

`Read/WriteStructInEEPROM()` functions should probably be called `Read/WriteStructFromEEPROM()` for proper english.
Author
Owner

fixed in a8be04b583

fixed in a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -0,0 +57,4 @@
// checks if EEPROM is ready
// If the dataset is 1, the datapoint is written at the address.
// If the dataset is 2 its written at another point.
void WriteDataPointInEEPROM(uint8_t dataPoint, uint8_t data){
Owner

what is dataPoint? is dataPoint a random byte? If so call it byte.

The data variable should probably be called fan_num or something. As it is it is not clear what data is.

The way I read this initially was the data was the data to prite and that dataPoint was maybe some pointer to where to write it.

what is dataPoint? is dataPoint a random byte? If so call it byte. The data variable should probably be called `fan_num` or something. As it is it is not clear what data is. The way I read this initially was the data was the data to prite and that dataPoint was maybe some pointer to where to write it.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -0,0 +62,4 @@
checkEEpromIsReady();
if (data == 1){
eeprom_update_byte(currentAddressFan1, dataPoint);
currentAddressFan1++;
Owner

Why are these global? Could they not be a local volatile variable?

Why are these global? Could they not be a local volatile variable?
Author
Owner

i agree but not shure if eemem allows it, will check

i agree but not shure if eemem allows it, will check
Author
Owner

i need the variables in write and read function so i do not think it should be local, if you do not have a solution to that problem?

i need the variables in write and read function so i do not think it should be local, if you do not have a solution to that problem?
Athamantis marked this conversation as resolved
@ -0,0 +67,4 @@
eeprom_update_byte(currentAddressFan2, dataPoint);
currentAddressFan2++;
} else{
// error???
Owner

A common way to return errors is to make the function an int.
An int returning 0 = OK. Anyting other than 0 indicates and error.
If you want to return an error this is the way to do it.

A common way to return errors is to make the function an int. An int returning 0 = OK. Anyting other than 0 indicates and error. If you want to return an error this is the way to do it.
Athamantis marked this conversation as resolved
@ -0,0 +74,4 @@
// Reads all the datapoints to the choosen data.
// it writes the data points in the USART stream.
void ReadDataPointSpeedInfo(uint8_t data){
Owner

This should return a pointer to the start of an array and the length of said array.
This allows us to do what we want to the data, not just print it.

Alternatively a function can be made later to output it over I2C, but returning a pointer and a length is a lot more flexible.

This should return a pointer to the start of an array and the length of said array. This allows us to do what we want to the data, not just print it. Alternatively a function can be made later to output it over I2C, but returning a pointer and a length is a lot more flexible.
Athamantis marked this conversation as resolved
@ -0,0 +1,50 @@
/*
* File: eeprom.h
* Author: athamantis
Owner

This should be your actual name, not your username.

This should be your actual name, not your username.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -4,15 +4,29 @@
*
* Created on March 6, 2024, 12:34 PM
*/
/*
Owner

We should not have two header sections.

Add your name to the list instead.

We should not have two header sections. Add your name to the list instead.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -10,0 +18,4 @@
#include <stdint.h>
#define F_CPU 4E6
#include <util/delay.h>
#include <avr/eeprom.h>
Owner

Is this needed in main? If unused it should be removed.

Is this needed in main? If unused it should be removed.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
@ -10,0 +20,4 @@
#include <util/delay.h>
#include <avr/eeprom.h>
#include <stdbool.h>
#define USART3_BAUD_RATE(BAUD_RATE) ((float)(F_CPU * 64 / (16 *(float) BAUD_RATE)) + 0.5)
Owner

Remove.

Remove.
Author
Owner

Fixed in commit a8be04b583

Fixed in commit a8be04b58358fc88b38f7e9431318e6a957cb0d9
Athamantis marked this conversation as resolved
Athamantis added 1 commit 2024-03-13 10:49:03 +00:00
Athamantis added 1 commit 2024-03-13 11:03:46 +00:00
Athamantis added 1 commit 2024-03-19 13:02:16 +00:00
Athamantis added 1 commit 2024-03-20 09:52:46 +00:00
Athamantis added 1 commit 2024-03-20 09:53:53 +00:00
Athamantis requested review from sebgab 2024-03-20 09:54:36 +00:00
sebgab approved these changes 2024-04-09 10:29:31 +00:00
sebgab left a comment
Owner

LGTM

LGTM
sebgab closed this pull request 2024-04-16 15:03:35 +00:00
sebgab deleted branch eeprom 2024-04-16 15:03:39 +00:00
sebgab reopened this pull request 2024-04-16 15:07:40 +00:00
sebgab closed this pull request 2024-04-16 15:09:41 +00:00
sebgab deleted branch eeprom 2024-04-16 15:10:09 +00:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mikrokontrollersystemer-gruppe-4/mikrokontrollersystemer-prosjekt#12
No description provided.