Rate my code

[lawrence11]

Code: [Select]

uint16_t prebufferk[4];
uint8_t bufferk[8];

typedef union {
uint16_t prebufferk[4];
uint8_t bufferk[8];
} combok;


uint8_t index_k;

const uint8_t index_k_ref=3;

extern USBD_HandleTypeDef hUsbDeviceHS;
int main(void)
{
  /* USER CODE BEGIN 1 */

  /* USER CODE END 1 */

  /* MCU Configuration--------------------------------------------------------*/

  /* Reset of all peripherals, Initializes the Flash interface and the Systick. */
  HAL_Init();

  /* USER CODE BEGIN Init */

  /* USER CODE END Init */

  /* Configure the system clock */
  SystemClock_Config();


  /* Initialize all configured peripherals */

  index_k=0;
  MX_GPIO_Init();
  MX_USB_DEVICE_Init();
  MX_SPI2_Init();
  MX_TIM2_Init();
  /* USER CODE BEGIN 2 */
  bufferk[0]=1;//reportID
  bufferk[1]=0;//modifier
  bufferk[2]=0;//OEM
  bufferk[3]=0;//keycode data
  bufferk[4]=0;//keycode data
  bufferk[5]=0;//keycode data
  bufferk[6]=0;//keycode data
  bufferk[7]=0;//keycode data
  /* USER CODE END 2 */

  /* Infinite loop */
  /* USER CODE BEGIN WHILE */
  while (1)
  {
    /* USER CODE END WHILE */
if(LL_SPI_IsActiveFlag_RXNE(SPI2))
{
if(LL_GPIO_IsInputPinSet(f7i1_GPIO_Port, f7i1_Pin) && index_k !=index_k_ref) // If pin is high and index threshhold not reached
{
prebufferk[index_k]=SPI2->DR;
index_k ++;
}
else
{

}
}

if(index_k==index_k_ref){
USBD_HID_SendReport(&hUsbDeviceHS, bufferk, 8);
}


    /* USER CODE BEGIN 3 */
  }
  /* USER CODE END 3 */
}



Code: [Select]

uint8_t USBD_HID_SendReport(USBD_HandleTypeDef  *pdev,
                            uint8_t *report,
                            uint16_t len)
{
  USBD_HID_HandleTypeDef     *hhid = (USBD_HID_HandleTypeDef *)pdev->pClassData;

  if (pdev->dev_state == USBD_STATE_CONFIGURED)
  {
    if (hhid->state == HID_IDLE)
    {
      hhid->state = HID_BUSY;
      USBD_LL_Transmit(pdev,
                       HID_EPIN_ADDR,
                       report,
                        len);
                        index_k=0; // This part does not compile.

    }
  }
  return USBD_OK;
}

So the goal is to transfer a SPI buffer into some ram and once the accompanying buffer is full, write to USB endpoint.

One problem I face face is that index_k in the HID_Sendreport function is not recognized, cant remember if I should put index_k extern and inside usbd_hid.c ?

[ataradov]

What is there to "rate"? It is a trivial amount of code.

If you are asking about part that does not compile, then having the test of the error would be helpful.

What is the relative location of those two pieces of code?

EDIT based on the update: It must be declared for real in one file and declared extern in all other files that use it. Or in a common header file.

[lawrence11]

What is there to "rate"? It is a trivial amount of code.

If you are asking about part that does not compile, then having the test of the error would be helpful.

What is the relative location of those two pieces of code?

EDIT based on the update: It must be declared for real in one file and declared extern in all other files that use it. Or in a common header file.

All compiled untill I decide to modify the HID_Sendreport and add index_k=0; at the end.

Static extern int8_t index_k;  inside usbd.c ??

Missing an #include somewhere.

[lawrence11]

What about my use of a union?

Is this legit?

I just wanna transfer the SPI register, wich works with 16bit SPI setup to minimize downtime duh.

Then pass this data to function, wich takes only bytes.

[ataradov]

The union itself is fine, but I don't understand what you are trying to do after that. This code only  declares a type, but never instantiate a variable of this type.

And if all you need is pass the array of uint16_t as an array of bytes, you can simply cast.

It can't be static and extern at the same time. No need for static in this case.

[lawrence11]

I messed up the constants values, it should be +1 offset from those numbers.

I just wanna write computer as keyboard via SPI bridge

Compiles now, zero erros, zero warnings.

I kinda needed help because I only got 1 st-link.

[AG6QR]

What about my use of a union?

Is this legit?

I just wanna transfer the SPI register, wich works with 16bit SPI setup to minimize downtime duh.

Then pass this data to function, wich takes only bytes.

When you make a union of different sized integer types, beware of bugs regarding big-endian vs. little-endian.  If you only want your code to run on one architecture, you can possibly get away with hacking it until it works, and not worry so much.  If you want it to be portable, you should write two versions of the critical portions of the code, and use #ifdef to select the proper version based on the endian-ness of the processor.

[TomS_]

Style wise, you have an inconsistent use of spaces around operators and what not.

this=that vs this = that
this ++
while (1)
if(...)

Based on that, low rating.

[lawrence11]

Thats because, theres was like a ton of other stuff that I deleted.

Not gonna put effort beyond what is readable.

[thinkfat]

Style wise, you have an inconsistent use of spaces around operators and what not.

this=that vs this = that
this ++
while (1)
if(...)

Based on that, low rating.

Yep, together with comments leftover from copying some code template which don't give any hint what the code is doing,
the inconsistent indentation, the mix of C and C++ comment style, not using initializers for seeding bufferk[], and on top if it,
IF IS NOT A FUNCTION CALL!!!11

1 out of 5, and the "1" is for spending the effort of showing up and asking for comments.

 

[mariush]

 nothing jumping out

i hate brackets on single lines  ex 
}
else
{

just type } else {

also

Code: [Select]

if(LL_GPIO_IsInputPinSet(f7i1_GPIO_Port, f7i1_Pin) && index_k !=index_k_ref) // If pin is high and index threshhold not reached
{
prebufferk[index_k]=SPI2->DR;
index_k ++;
}
else
{

}
could be rewritten as

Code: [Select]

if(LL_GPIO_IsInputPinSet(f7i1_GPIO_Port, f7i1_Pin) && index_k != index_k_ref) {
// -comment on new line inside the brackets , indentation makes it obvious where the code belongs
// If pin is high and index threshhold not reached
prebufferk[index_k]=SPI2->DR;
index_k ++;
} else {
// the else part
}

[donotdespisethesnake]

IF IS NOT A FUNCTION CALL!!!11

That is true, but why do you point that out?

[SiliconWizard]

Thats because, theres was like a ton of other stuff that I deleted.

Not gonna put effort beyond what is readable.

Then don't ask people to rate your code.
(Oh and don't overestimate the readability of a given piece of code, see below.)

Consistency is a key point for readability and maintenance, much more so than style itself, which can have almost as many variants as there are developers (and it often leads to completely fruitlesss discussions). So pick a style and use it consistently. Lack of consistence just looks sloppy and kills readability.

Other than that, you're using some global variables like index_k which IMO shouldn't be global variables. Keep that local. Pass it to other functions as a parameter if needed.
Apart from specific cases, favor parameters to functions instead of relying on a bunch of global variables which will just make your code look like spaghetti and will make your functions non-reentrant.

[thinkfat]

IF IS NOT A FUNCTION CALL!!!11

That is true, but why do you point that out?

Right:

Code: [Select]

if (expression)WRONG:

Code: [Select]

if(expression)

[Siwastaja]

WRONG:

Code: [Select]

if(expression)

You must be new to programming. Don't worry, most of the too-simplistic-to-be-meaningful arrogance that leads to such false statements of perfectly correct code being "wrong" will go away once you have to actually work on code made by others possibly using different styles.

[lawrence11]

Thats because, theres was like a ton of other stuff that I deleted.

Not gonna put effort beyond what is readable.

Then don't ask people to rate your code.
(Oh and don't overestimate the readability of a given piece of code, see below.)

Consistency is a key point for readability and maintenance, much more so than style itself, which can have almost as many variants as there are developers (and it often leads to completely fruitlesss discussions). So pick a style and use it consistently. Lack of consistence just looks sloppy and kills readability.

Other than that, you're using some global variables like index_k which IMO shouldn't be global variables. Keep that local. Pass it to other functions as a parameter if needed.
Apart from specific cases, favor parameters to functions instead of relying on a bunch of global variables which will just make your code look like spaghetti and will make your functions non-reentrant.

I dont understand the problem with

if(LL_SPI_IsActiveFlag_RXNE(SPI2)) vs while(LL_SPI_IsActiveFlag_RXNE(SPI2))

I thought the only difference was that after the loop the if doesnt check if true again.

I googled these exact terms : LL_SPI_IsActiveFlag_RXNE(SPI2) , I always saw it in a while and sometimes if.

If you XXXXXXX=SPI2->DR; , its cleared, then forks again, doest it always = the same time execution but if is safer for overall SW?

[thinkfat]

WRONG:

Code: [Select]

if(expression)

You must be new to programming. Don't worry, most of the too-simplistic-to-be-meaningful arrogance that leads to such false statements of perfectly correct code being "wrong" will go away once you have to actually work on code made by others possibly using different styles.

You must be an EE.  That's fine. Just don't try to teach me "programming".

[AG6QR]

I dont understand the problem with

if(LL_SPI_IsActiveFlag_RXNE(SPI2)) vs while(LL_SPI_IsActiveFlag_RXNE(SPI2))

I thought the only difference was that after the loop the if doesnt check if true again.

The difference is that an if is not a loop.  A while is a loop.  Thus, there is no "after the loop" with the if block.

In general, comments shouldn't put much effort into explaining what your code is doing, but instead they should explain why. It is almost never appropriate to explain basic language features in comments.  You can assume the programmer knows the language, or at least knows where to find a manual to fill in any gaps in language knowledge.

The reason for comments is to tell someone, many years in the future, what they need to know in order to understand, debug, maintain, and enhance your code. 

The programmer who reads your comments might be your future self, so write them well!

[lawrence11]

Did you notice above I wrote

''I thought the only difference was that after the loop the if doesnt check if true again.''

All you guys do is jackoff around and complain for no reason, typical programmers.

[NivagSwerdna]

You are brave.

If I was going to be fussy...

In days of old when knights roamed the land chasing damsels we might have used lint.... It probably would have said that any constants other than 0 or 1 should be #defined.

I don't particularly like the buffer intialisation style but it is harmless and I don't like the inconsistent indentation around {}...

... But if it works then go for it.

[donotdespisethesnake]

IF IS NOT A FUNCTION CALL!!!11

That is true, but why do you point that out?

Right:

Code: [Select]

if (expression)WRONG:

Code: [Select]

if(expression)

  

This is why code reviews are such a waste of time. People argue over complete trivia, and rarely find any real bugs.

[lawrence11]

Thats because, theres was like a ton of other stuff that I deleted.

Not gonna put effort beyond what is readable.

Then don't ask people to rate your code.
(Oh and don't overestimate the readability of a given piece of code, see below.)

Other than that, you're using some global variables like index_k which IMO shouldn't be global variables. Keep that local. Pass it to other functions as a parameter if needed.
Apart from specific cases, favor parameters to functions instead of relying on a bunch of global variables which will just make your code look like spaghetti and will make your functions non-reentrant.

Heres the thing tho, I`m a bit scared to touch these functions, messing with their parameters.

So you think its worth it??

[newbrain]

[...]Don't worry, most of the too-simplistic-to-be-meaningful arrogance that leads to such false statements of perfectly correct code being "wrong" will go away once you have to actually work on code made by others possibly using different styles.

This.

I see most of the comments focused on formatting, indentation, spaces and similar trivia. I did not have time to go through the actual code.

I will simply say: use a modern editor of your choice, they all can indent code correctly; this will also help catch a number of coding mistakes.
Some are better than other, but like indentation, everyone has their tastes.
If you are working in a team, stick to the existing convention (even if you personally dislike it).

[thinkfat]

 

This is why code reviews are such a waste of time. People argue over complete trivia, and rarely find any real bugs.

Yes, but the OP was not asking for a code review but for a "rating". My comments were in the spirit of that, on the "style". Which is sort of a useless metric, unless you're in the business of writing "safe" code that is subject to certification, requiring enforcing of certain coding standards.

Though, as you progress as a software developer, you'll find it necessary to develop a certain consistency and you'll learn that it really helps to place "hints" in the code for your brain to parse it more quickly. Discerning strictly between function calls and structural and control elements is one such hint that helps you along.

[jbee]

So you think its worth it??

If you're working in a bare-metal environment with no OS or threading and you know what you're doing (ie if your global sits in an interrupt, do you know when it's going to get set/read), having globals is fine.  You're taught to limit the scope of objects as best as you can, but in these small microcontroller projects, sometimes having globals is more readable than following a stupid counter parameter through function calls, etc.  In fact by having a global instead of passing it around, you avoid the dynamic stack memory allocation.  In my small microcontroller projects, clock ticks are more valuable than following rules for the sake of following rules.

All that being said, I don't think you're code is going to work because of that union.  You declare combok as a union of a uint16_t and uint8_t, but you never declare a combok object.  So in all those places you're setting the  bufferk variable and expecting  prebufferk to be sitting at the same memory location, you're wrong.  Because you declared bufferk and prebufferk above the union, those are the variables you're *actually* setting.  They're not tied together at all, they're just separate variables.  So when you do prebufferk[index_k]=SPI2->DR; you're setting that register to garbage, or 0, or however the compiler initializes globals.  Sorry, I read that backwards, you're saving the DR register to prebufferk.  It still stands that if you think there's a relationship between bufferk and prebufferk, there's not; but I'm not 100% sure if that's what you're trying to do.