Sensored BLDC Commutation Controller code

Question

For context, I'm using a maxII epm240 cpld, and I want to be able to program this to FPGAs too. I know I could build this in a typical state machine fashion since a sensored BLDC motor controller is a Mealy state machine. But, I figured that since I can use the motor as the clock to facilitate state updating, all I needed was a combinational logic to decide how the Commutation Controller works.

This combinatorial logic works, and I have built a motor controller out of it. I was just wondering whether this type of way of building a Mealy state machine is problematic or if there's any way I could improve this code.

Here is the link to my github to see more details:

https://github.com/SimNabong/Sensored-Brushless-DC-Motor-Controller/tree/main

module CommutationControl(
    input clk,
    input [2:0]UI, 
    input [2:0]HS, //Hall Sensors 
    output [5:0]PT //6 Power Transistor control signals
);
    /*
      UI[0](Regen Break One) or UI[1]&UI[2](Regen Break Two)
      UI[1] is clockwise spin
      UI[2] is counter-cw spin
      HS[0],HS[1],HS[2] are the HS sensor signals
    */  
    
    wire Aw,Bw,Cw,Dw,Ew,Fw;
    reg [1:0]Ar = 2'd0;
    reg [1:0]Br = 2'd0;
    reg [1:0]Cr = 2'd0;
    reg [1:0]Dr = 2'd0;
    reg [1:0]Er = 2'd0;
    reg [1:0]Fr = 2'd0; //registers for delays


    assign Aw = ~HS[0]&HS[2]&~UI[0]&UI[2] | ~HS[1]&HS[2]&~UI[0]&UI[1] | ~HS[0]&HS[1]&~UI[0]&UI[1]&UI[2] | HS[0]&~HS[1]&~UI[0]&UI[1]&UI[2] | HS[0]&~HS[2]&~UI[0]&UI[1]&UI[2] | HS[1]&~HS[2]&~UI[0]&UI[1]&UI[2];
    
    assign Bw = HS[1]&~HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[1]&HS[2]&UI[0]&~UI[1]&~UI[2] | HS[0]&~HS[2]&UI[0]&~UI[1]&~UI[2] | HS[0]&~HS[2]&~UI[0]&~UI[1]&UI[2] | ~HS[0]&HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[1]&UI[0]&~UI[1]&~UI[2] | HS[1]&~HS[2]&~UI[0]&UI[1]&~UI[2] | HS[0]&~HS[1]&UI[0]&~UI[1]&~UI[2];
    
    assign Cw = HS[0]&~HS[1]&~UI[0]&UI[1]&UI[2] | ~HS[1]&HS[2]&~UI[0]&UI[1]&UI[2] | ~HS[0]&HS[1]&~UI[0]&UI[1]&UI[2] | HS[1]&~HS[2]&~UI[0]&UI[2] | HS[0]&~HS[2]&~UI[0]&UI[1] | ~HS[0]&HS[2]&~UI[0]&UI[1]&UI[2];
    
    assign Dw = ~HS[0]&HS[1]&UI[0]&~UI[1]&~UI[2] | HS[0]&~HS[1]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[1]&HS[2]&~UI[0]&~UI[1]&UI[2] | ~HS[1]&HS[2]&UI[0]&~UI[1]&~UI[2] | HS[1]&~HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[2]&~UI[0]&UI[1]&~UI[2] | HS[0]&~HS[2]&UI[0]&~UI[1]&~UI[2];
    
    assign Ew = ~HS[0]&HS[2]&~UI[0]&UI[1]&UI[2] | HS[0]&~HS[2]&~UI[0]&UI[1]&UI[2] | HS[0]&~HS[1]&~UI[0]&UI[2] | ~HS[1]&HS[2]&~UI[0]&UI[1]&UI[2] | HS[1]&~HS[2]&~UI[0]&UI[1]&UI[2] | ~HS[0]&HS[1]&~UI[0]&UI[1];
    
    assign Fw = HS[0]&~HS[1]&~UI[0]&UI[1]&~UI[2] | HS[0]&~HS[1]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[1]&~UI[0]&~UI[1]&UI[2] | HS[0]&~HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[1]&UI[0]&~UI[1]&~UI[2] | ~HS[1]&HS[2]&UI[0]&~UI[1]&~UI[2] | ~HS[0]&HS[2]&UI[0]&~UI[1]&~UI[2] | HS[1]&~HS[2]&UI[0]&~UI[1]&~UI[2];
    
    
    always@(posedge clk)begin  //introduces a dead-time that prevents power transistors in the same bridge from being on at the same time, because if they did then that would cause a short in  the battery. This part however can be removed if the gate drivers has this capability. This dead-time needs to be adjusted based on the turn off/on delay of the power trans used.
    
        Ar[0] <= Aw&~Br[1];
        Ar[1] <= Ar[0]; 
        Br[0] <= Bw&~Ar[1]; 
        Br[1] <= Br[0];
        
        
        Cr[0] <= Cw&~Dr[1];
        Cr[1] <= Cr[0];
        Dr[0] <= Dw&~Cr[1];
        Dr[1] <= Dr[0];
        
        Er[0] <= Ew&~Fr[1];
        Er[1] <= Er[0];
        Fr[0] <= Fw&~Er[1];
        Fr[1] <= Fr[0];     
        
    end
    

    assign PT = {Ar[1],Br[1],Cr[1],Dr[1],Er[1],Fr[1]};



endmodule

Answer 1

Overview

It is good that you did the following:

• Chose a meaningful name for the module
• Used ANSI-style module ports instead of the older style
• Declared one port per line for better readability
• Added comments
• Used proper and consistent indentation
• Used nonblocking assignments in the sequential logic always block

Here are some suggestions for improvement.

Long lines

The assign statements are much too long for a single line. Even with the large screen I am using, I need to scroll horizontally too much. This impedes understanding of the design. You can easily break up each single line into many lines. For example:

assign Aw = ~HS[0]&HS[2]&~UI[0]&UI[2] | 
            ~HS[1]&HS[2]&~UI[0]&UI[1] | 
            ~HS[0]&HS[1]&~UI[0]&UI[1]&UI[2] | // etc.

The same is true for the comment after the always block. You should break that up into several lines inside a "block" comment. For example:

/*
    Introduces a dead-time that prevents power transistors in the same bridge
    from being on at the same time, because if they did,
    then that would cause a short in the battery.
    etc.
*/
always@(posedge clk)begin  

Vertical whitespace

The code would be easier to understand if there were more of it to read on the screen at once. You should collapse all multiple blank lines into a single blank line:

    always@(posedge clk)begin    
        Ar[0] <= Aw&~Br[1];
        Ar[1] <= Ar[0]; 
        Br[0] <= Bw&~Ar[1]; 
        Br[1] <= Br[0];
        
        Cr[0] <= Cw&~Dr[1];
        Cr[1] <= Cr[0];
        Dr[0] <= Dw&~Cr[1];
        Dr[1] <= Dr[0];
        
        Er[0] <= Ew&~Fr[1];
        Er[1] <= Er[0];
        Fr[0] <= Fw&~Er[1];
        Fr[1] <= Fr[0];     
    end
    
    assign PT = {Ar[1],Br[1],Cr[1],Dr[1],Er[1],Fr[1]};
endmodule

Horizontal whitespace

Code is easier to read with space around some operators. For example:

    Ar[0] <= Aw & ~Br[1];

It is good to have space around signal ranges:

input [2:0] UI, 

It is good to use space after commas:

assign PT = {Ar[1], Br[1], Cr[1], Dr[1], Er[1], Fr[1]};

Also use space around keywords:

always @(posedge clk) begin

Naming

Use meaningful names for the signals. For example, instead of an abbreviation like HS, use words like hall_sensors. With a descriptive name like this, the comments become redundant and can be removed. Using proper naming makes the code more self-documenting.

It is more customary to use all lower-case letter for signal names. All caps is typically used for constant values, like a parameter.

PT can be power_transistors.

I have no idea what UI means. There is a good chance that you will even forget what it means a year from now when you look back at your code. You could consider spitting this input into 3 separate signals with meaningful names. For example, UI[2] could be ccw_spin.

Naming the wires and regs A-F is not very meaningful. If the letters do have some meaning, you should at least add comments describing what they stand for.

Reset

It is common for digital logic to use a dedicated input signal to reset the design. Perhaps this is not required for your FPGA, but it is worth considering to make the code more portable. For example, initializing registers this way may not be synthesizable with other design flows:

reg [1:0]Ar = 2'd0;

State machine

I would not classify the module as any type of state machine. In Verilog, a state machine is typically coded with a state register, parameter constants for each state, and a case statement to describe transitions between the states. Here is an example of an FSM. Perhaps your Mealy FSM is implemented in another module.

Abstraction

Seeing all those sum-of-products (like ~HS[0]&HS[2]&~UI[0]&UI[2]) makes me wonder if you are coding at too low a level of abstraction. It is not evident to me how code this at a higher level because I don't understand the design, but it is something for you to consider.