User Tools

Differences

This shows you the differences between two versions of the page.

Link to this comparison view

Both sides previous revisionPrevious revision
Next revision		Previous revision
Next revisionBoth sides next revision
user:embedded_c_code_doesn_t_have_to_be_ugly [2020/05/11 17:18] – [4. Bit manipulations vs. structured data] Igor Yefmovuser:embedded_c_code_doesn_t_have_to_be_ugly [2020/05/11 18:27] – [5. Globals] Igor Yefmov
Line 138: Line 138:
 const uint8_t * const location_arr = pageAddress.a;</code> const uint8_t * const location_arr = pageAddress.a;</code>
  
 +=== A much more "complicated" example ===
 +Incoming USB requests have a standard "setup" header which results in old-style code looking something like the following((there's a separate discussion on the use of globals later on)):
 +<code C>static CyBool_t CyFxUVCApplnUSBSetupCB(uint32_t setupdat0, /* SETUP Data 0 */
 +                                       uint32_t setupdat1 /* SETUP Data 1 */
 +) {
 +    CyU3PIoMatrixConfig_t io_cfg;
 +    CyBool_t uvcHandleReq = CyFalse;
 +    uint32_t status;
 +    CyU3PReturnStatus_t apiRetStatus = CY_U3P_SUCCESS;
 +    uint8_t address, data;
  
 +    /* Obtain Request Type and Request */
 +    bmReqType = (uint8_t) (setupdat0 & CY_FX_USB_SETUP_REQ_TYPE_MASK );
 +    bRequest = (uint8_t) ((setupdat0 & CY_FX_USB_SETUP_REQ_MASK ) >> 8);
 +    bType = (bmReqType & CY_U3P_USB_TYPE_MASK);
 +    bTarget = (bmReqType & CY_U3P_USB_TARGET_MASK);
 +    wValue = (uint16_t) ((setupdat0 & CY_FX_USB_SETUP_VALUE_MASK ) >> 16);
 +    wIndex = (uint16_t) (setupdat1 & CY_FX_USB_SETUP_INDEX_MASK );
 +    wLength = (uint16_t) ((setupdat1 & CY_FX_USB_SETUP_LENGTH_MASK ) >> 16);
 +
 +    /* Handle USB vendor requests */
 +    if (bType == CY_U3P_USB_VENDOR_RQT) {
 +</code>
 +The function goes on for HUNDREDS of lines of deep nesting!!!((over 600 LoC to handle the standard USB requests)).
 +
 +Don't know about you, but all this bit shifting and byte extraction just screams "fruitful field for errors!" to me. Consider having abstracted all this ugliness into a structure like the following:
 +<code C>enum UsbRequestTarget{usb_tgt_dev, usb_tgt_ifc, usb_tgt_ep, usb_tgt_other};
 +enum UsbRequestType{usb_rt_std, usb_rt_class, usb_rt_vendor, usb_rt_reserved};
 +typedef struct UsbSetupPacket{
 +    union{
 +        uint32_t d0;
 +        struct{
 +            struct{
 +                struct{
 +                    enum UsbRequestTarget bTarget : 5;
 +                    enum UsbRequestType bType : 2;
 +                    uint8_t dirIn : 1; // "IN" is the direction towards USB host (reading data)
 +                };
 +                uint8_t bRequest;
 +            };
 +            union{
 +                PackedUint16 wValue;
 +                uint16_t featureSelector;
 +            };
 +        };
 +    };
 +    union{
 +        uint32_t d1;
 +        struct{
 +            union{
 +                PackedUint16 wIndex;
 +                struct{
 +                    union{
 +                        uint8_t all;
 +                        struct{
 +                            uint8_t num : 4;
 +                            uint8_t res3 : 3;
 +                            uint8_t dirIn : 1;
 +                        };
 +                    };
 +                    uint8_t res8 : 8;
 +                } ep;
 +                struct Infc{
 +                    uint8_t num;
 +                    uint8_t res;
 +                } infc;
 +            };
 +            PackedUint16 wLength;
 +        };
 +    };
 +} UsbSetupPacket;
 +</code>
 +
 +With that little prep work the same code becomes so much more readable and maintainable:
 +<code C>static CyBool_t CyFxUVCApplnUSBSetupCB(uint32_t setupdat0, uint32_t setupdat1)
 +{
 +    UsbSetupPacket req = {};
 +    // the next 2 lines is all it takes to distribute the bits into proper NAMED variables
 +    req.d0 = setupdat0;
 +    req.d1 = setupdat1;
 +
 +    // dispatch to appropriate handler based on request type and return status of request handling to the USB driver
 +    switch(req.bType){
 +        case usb_rt_vendor: return handleVendorRqt(&req);
 +        case usb_rt_std:    return handleUsbStdRqt(&req);
 +        case usb_rt_class:  return handleUvcRqt(&req);
 +        default: return CyFalse;
 +    }
 +}
 +</code>
 +
 +Not only the code becomes that much more readable - it also opens up opportunities for compilers to do platform-specific optimizations which are otherwise just not available to them as the intent of the code is not clearly expressed with all that manual bit shifting...
 ==== 5. Globals ==== ==== 5. Globals ====
 +Having global variables is absolutely the major contributor to entangling unrelated blocks of code and making sure no human mind can fully comprehend your text without first truly understanding **all of it**. "A variable is declared in one file, set in another, accessed in yet another... what fun it is to trace its lifetime!" especially on an embedded system that lacks "usual" debugging tools like stepping through the lines or examining the state of variables.
 +
 +There are a few kinds of globals, IMO:
 +=== "Global" globals ===
 +These are the ones declared at file level and mentioned with an ''extern'' keyword in header files. In my experience almost all of these can be safely made non-globals with little to no impact on the overall code structure and quite often moved into function scopes and passed around as parameters on a stack((more on stack usage later)).
 +
 +See if some of those globals can be made file-scoped by grouping related functions together((naturally all the functions that access the same set of globals are very likely to be related to each other in functionality)). Some ''const''((some are not even declared as ''const'' at times!)) globals can even be turned into ''enum'''s((and stop taking precious RAM or ''.data'' (sometimes ''.text'') segment space)). And most often those globals can, and should, be safely made function-scope.
 +
 +=== Function-level globals ===
 +A relic of K&R style ''C'' language, that required all the function variables be declared up-front, still finds its proponents even in the modern world of not-dumb-anymore-compilers that are able to deduce a lot more information from the code's text than some 50 years ago.
 +
 +These same compilers are capable of making quite intelligent choices on how to translate your code into zeroes and ones, so that you should truly be not afraid of expressing your functionality in the language of abstraction, rather than writing in ''C'' like it is a macro-Assembler((I'm absolutely not advocating for total ignorance on how your ''C'' code translates into machine code, of course not. Just saying that one must **not** try to "help" compilers too much at expense of code's clarity)).
 +
 +Stack allocation is extremely cheap, even on the most low-end down-to-earth hardware, so concerning yourself with the cost of that operation by grouping all-the-possible-function-variables together at the top should be reserved only for the most critical code((in which case it is very likely that such code should be written directly in Assembler by a highly-skilled engineer who is also extremely familiar with the target platform)).
 +
 +=== Cross-function globals ===
 +Oh, these are absolutely the worst, as they tend to sneak up on you at the least expected time and place! They get declared as globals and then are only set in one place while being accessed in plenty of other places. Especially dangerous ones cross thread boundaries, making for extra-nasty race condition type bugs.
 +
 +Even if one doesn't do anything about the first two kinds, one must always strive to eliminate these ones. In the long run it **never** makes sense to have that kind of (questionable) optimization benefit at the expense of destabilizing your code so badly.
  
 ==== 6. Code blocks' nesting ==== ==== 6. Code blocks' nesting ====

This website uses cookies. By using the website, you agree with storing cookies on your computer. Also, you acknowledge that you have read and understand our Privacy Policy. If you do not agree, please leave the website.

More information