Jump to content
 

All Activity

This stream auto-updates     

  1. Last week
  2. I have been told many times that we should avoid the use of magic numbers in our code. That is numbers like these: CMCON0 = 0x5d; for(x = 0; x < 29; x ++) { // do something clever with x } The rational is that we don't really know what 0x5D means when it is written to CMCON0 or why we should be stopping at 28 in the for loop. However if you made the code like this: #define CMCON0_PWM_CONFIGURATION <PWM_MAGIC_NUMBERS> // See Datasheet Chapter 3.4 on PWM configuration #define MAXIMUM_PLAYER_COUNT 29 CMCON0 = CMCON0_PWM_CONFIGURATION; for(x = 0; x < MAXIMUM_PLAYER_COUNT; x ++) { // do something with each player } Clearly this code is much more readable. However, there is a time when this practice is simply crazy. For example, If you only configure the PWM in a single place, you may not want to hunt around for the exact value placed in CMCON because you have the datasheet open in front of you so the right place for the exact value is right there on that line. Putting the value in one place to use in another place just makes more work. Or what about this example: // Found in a configuration header file #define ZERO_VALUE 0 #define ONE_VALUE 1 #define TWO_VALUE 2 #define THREE_VALUE 3 #define FOUR_VALUE 4 #define FIVE_VALUE 5 #define SIX_VALUE 6 #define SEVEN_VALUE 7 // Found later in the C code switch(device_number) { case ZERO_VALUE: <blah blah blah> break; case ONE_VALUE: <ETC> } I would LOVE a rational explanation on why doing this is little more than cargo culting a coding style. I have seen this sort of sillyness in many other places. // is this better? delay_ms(1000); // or this delay_ms(ONE_SECOND); // or perhaps this inline void delay_One_Second(void) { delay_ms(1000); } At the end of the day, I want to see numbers where they make sense. The MOST important reason for using the NO_MAGIC_NUMBER pattern is really NOT to clarify the numbers, but really to make the code DRY. For example consider this: #define MESSAGE_TIMEOUT 1000 // later delay_ms(MESSAGE_TIMEOUT); // or even better void delay_message_timeout(void); If you are going to use the MESSAGE_TIMEOUT in a large number of places and especially if you may be tweaking the value, then putting the number in one place is only sane. But the BEST way is to make the function because that is the MOST clear when you read the final code and it creates a very nice point of abstraction for any number of delay methods. The biggest argument FOR the anti-magic number pattern is really about documentation. In a simple way, the proponents of this pattern are trying to follow the "comments are a smell" pattern but usually without realizing it. In this way, a standin for the number can be used that makes the purpose clear. This is a powerful point and should be considered carefully. But, sometimes, the clearest value to put in the code is actually the MAGIC number. AND, if you don't understand that number, you have no business editing that part of the code. As always, be prudent and understand the needs of the programmers who will follow. That programmer is likely to be you. Good Luck.
  3. I am building an integrated audio interface for a Baofeng UV-5R hand-held radio. Primarily this is to get a packet radio APRS system up and running. Traditionally, this seems to be accomplished with a crazy collection of adapters and hand crafted interface cables. As I was looking for a better solution, I discovered the Teensy family of ARM microcontrollers. (actually Arduino high performance ARM's built on NXP Cortex M4's) The important part is actually the library support. Out of the box I was able to get a USB Audio device up and map the ADC and DAC to the input and output. This could all be configured with some simple "patch cord" wiring. So I created the "program" above. This combines the 2 audio channels from USB into a single DAC and also passes the data to an RMS block. This causes the audio to play on the DAC at 44.1khz. It also keeps a running RMS value available for your own code. More on this later. Next the ADC data is duplicated into both channels back through USB and on to the PC (Raspberry Pi). Again an RMS block is present here as well. Now pressing EXPORT produces this little block of "code" Which gets pasted into the Arduino IDE at the beginning of the program (before your setup & main. /* * A simple hardware test which receives audio on the A2 analog pin * and sends it to the PWM (pin 3) output and DAC (A14 pin) output. * * This example code is in the public domain. */ #include <Audio.h> #include <Wire.h> #include <SPI.h> #include <SD.h> #include <SerialFlash.h> #include <Audio.h> #include <Wire.h> #include <SPI.h> #include <SD.h> #include <SerialFlash.h> // GUItool: begin automatically generated code AudioInputUSB usb1; //xy=91,73.00001907348633 AudioInputAnalog adc1; //xy=153.00000381469727,215.00003242492676 AudioMixer4 mixer1; //xy=257.00000762939453,72.00002670288086 AudioAnalyzeRMS rms2; //xy=427.0000114440918,266.000036239624 AudioOutputUSB usb2; //xy=430.00001525878906,217.00003242492676 AudioOutputAnalog dac1; //xy=498.00009536743164,72.00002670288086 AudioAnalyzeRMS rms1; //xy=498.00001525878906,129.0000295639038 AudioConnection patchCord1(usb1, 0, mixer1, 0); AudioConnection patchCord2(usb1, 1, mixer1, 1); AudioConnection patchCord3(adc1, 0, usb2, 0); AudioConnection patchCord4(adc1, 0, usb2, 1); AudioConnection patchCord5(adc1, rms2); AudioConnection patchCord6(mixer1, dac1); AudioConnection patchCord7(mixer1, rms1); // GUItool: end automatically generated code const int LED = 13; void setup() { // Audio connections require memory to work. For more // detailed information, see the MemoryAndCpuUsage example AudioMemory(12); pinMode(LED,OUTPUT); } void loop() { // Do nothing here. The Audio flows automatically if(rms1.available()) { if(rms1.read() > 0.25) { digitalWrite(LED,HIGH); } else { digitalWrite(LED,LOW); } } // When AudioInputAnalog is running, analogRead() must NOT be used. } And Viola!. The PC sees this as an audio device and the LED blinks when the audio starts. PERFECT. Now, the LED will be replaced with the push to talk (PTT) circuit and the audio I/O will connect to the Baofeng through some filters. A single board interface to the radio from a Raspberry Pi that does not require 6 custom cables, and a 3 trips to E-BAY. Now I am waiting for my PCB's from dirtypcb.com This entire bit of work is for my radio system that is being installed on the side of my house. Here is the box: Inside is a Raspberry Pi 3, a Baofeng UV-5R for 2M work, 3 RTL dongles for receiving 1090MHz ADS-B, 978MHz ADS-B, 137MHz Satellite weather, GPS and 1 LoRaWAN 8 channel Gateway. I will write more about the configuration later if there is any interest. Good Luck.
  4. Earlier
  5. The driver is more like a framework for you to inject your own functions to change the behavior. The framework is pretty solid but the injected stuff is not complete (as you have discovered). The way to do that is to add a counter to the code we just did. When you start a transaction set it to X and count down on every retry. If you enable the NAK polling callback that will cause a retry, so you just have to modify that address nak function to retry only a limited number of times and that would be it 🙂
  6. Success!!! So the code no longer gets hung up on a NACK. Woot! Now I'm gonna try to figure out how to have the code retry a set number of times before it gives up and moves on.
  7. Ok, it is never that easy right :) Couple of small oversights up to now. 1. The do_I2C_SEND_STOP did not clear the busy flag 2. My code I posted above has a bug, doing a post-decrement and checkign for 0 later, which does not work. 3. NAK polling is enabled without any timeout, we need to disable this. After fixing these 3 things it should work as expected. This is how: 1. There is a bug in the driver at do_I2C_SEND_STOP, change that to look like this. It is setting the state to idle here but missing the clearing of the busy flag. static i2c_fsm_states_t do_I2C_SEND_STOP(void) { i2c1_driver_stop(); i2c_status.busy = 0; return I2C_IDLE; } 2. There is a bug in the code I posted above, I was doing to--, and after that checking for 0, but the post-decrement will reduce to to -1 by the time I was checking, so that should be like the following, main difference being : inline int mssp1_waitForEvent(uint16_t *timeout) { uint16_t to = 50000; // This will cause a 50ms timeout on I2C if(PIR3bits.SSP1IF == 0) { while(--to) { if(PIR3bits.SSP1IF) break; __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms } if (to == 0) { return -1; } } return 0; } 3. At this point the driver will operate in the default mode which is NAK Polling, this means that every time we get a NAK the driver will retry, assuming that the device we are talking to is busy, and we need to retry when a NAK happens. This is activated by setting the addressNAK callback to do a restart-write. We do not want this for your case, we want to fail and return when there is no answer. We can do this by commenting out the line in i2c_simple_master which enables this as follows: void i2c_write1ByteRegister(i2c_address_t address, uint8_t reg, uint8_t data) { while(!i2c_open(address)); // sit here until we get the bus.. i2c_setDataCompleteCallback(wr1RegCompleteHandler,&data); i2c_setBuffer(&reg,1); //i2c_setAddressNACKCallback(i2c_restartWrite,NULL); //NACK polling? i2c_masterWrite(); while(I2C_BUSY == i2c_close()); // sit here until finished. } Once you have done these three things the driver should do what you need, it will go back to idle after an address NAK with i2c_status.error set to I2C_FAIL.
  8. So there seems to be a couple of bugs here, firstly that it is not realizing that the address was a NAK, that is pretty standard and should work correctly, but after that it switches the command from write to read, which is also not correct. I would expect it to retry, but it does not make sense to retry using read if the initial attempt was to write to the device... I don't know how soon I will have time to debug this so here are some tips for you on how the driver works. The state machine and all decisions happen in i2c_masterOperation which is in i2c_master.c. In that function it will check if you are using the interrupt driven driver, in which case it will just return and let the state change happen in the IRQ, else it will run the poller, which will poll for the change in state and run the interrupt if the state change has happened. Essentially the poller is just polling the interrupt flag SSP1IF and calling the ISR when this is set. So there are 2 possible ways this could be going wrong. Case 1 is if the IF is never set, you can check this in the debugger but from what you describe it is not timing out which means that the IF seems to be set when the NAK happens. The other option is that NAK check is disabled or not successfully checked in the ISR. You should check here : void i2c_ISR(void) { mssp1_clearIRQ(); // NOTE: We are ignoring the Write Collision flag. // the write collision is when SSPBUF is written prematurely (2x in a row without sending) // NACK After address override Exception handler if(i2c_status.addressNACKCheck && i2c1_driver_isNACK()) { i2c_status.state = I2C_ADDRESS_NACK; // State Override } i2c_status.state = fsmStateTable[i2c_status.state](); } That if check there should be true. From what I can see, if you are not seeing a timeout, you must be seeing this ISR function being called from the poller, and this must set the state to I2C_ADDRESS_NACK, if this does not happen we can investigate if it is because the check is disabled or the ACKSTAT register has the wrong value. If it goes in there and the state is set the next place to look is where the NAK is processed, which should be here: // TODO: probably need 2 addressNACK's one from read and one from write. // the do NACK before RESTART or STOP is a special case that a new state simplifies. static i2c_fsm_states_t do_I2C_DO_ADDRESS_NACK(void) { i2c_status.addressNACKCheck = 0; i2c_status.error = I2C_FAIL; switch(i2c_status.callbackTable[i2c_addressNACK](i2c_status.callbackPayload[i2c_addressNACK])) { case i2c_restart_read: case i2c_restart_write: return do_I2C_SEND_RESTART(); default: return do_I2C_SEND_STOP(); } }
  9. I'm using a PICKit 4 for programming and debugging. I am using 2k ohm pull-ups. The transitions look nice and crisp, no ramp-ups when the bus is released. For debugging I have the I2C speed set to 100kHz, but everything still works at 1MHz. As you can see in the image below, the slave is sending ACK when everything is peachy. So the question now is how is the code reacting to a NACK. What I've noticed is, after the first byte is transmitted and there is nothing there to respond back to the master, the master then sends the next byte, and all subsequent bytes endlessly, interestingly the value of these bytes is the value of the first byte, plus one, as if the master is requesting a read operation. I'll debug and see if I can further understand the behavior and operation of the I2C driver, specifically what it's doing to monitor ACK and NACK.
  10. Yes that is what I meant when I said "I should actually check why you are not getting one". I would expect that you would send out the address and receive back a NAK when there is nothing connected to the bus. It would be important that you do have the pull-up resistors on the bus though. Looks like I am going to have to crack out that hardware and look at the signals on my Saleae to see what exactly is going on. I left my Saleae at the office so I can only do that tomorrow. Stand by and I will let you know what I find.
  11. So I made the code changes. When there is nothing on the I2C bus, the driver still goes into an endless loop. When I debug I notice that it never gets to time out, or rather the variable "to" never gets to count down from 50000. I am guessing PIR3bits.SSP1IF interrupt flag gets set and it breaks out of the mssp1_waitForEvent function. But if there is nothing on the bus why is this flag setting? Is this the correct interrupt to be monitoring? I don't know enough to know if I'm asking the right questions, but I am thinking that if the Master device on the I2C bus sends out an address and does not receive an ACK from the slave, shouldn't a time-out routine be looking for that slave's acknowledgement? Do I have that right?
  12. Ok this is slightly more complex because the status is one layer up so you have to pass the fact that there was a timeout up one layer, but that function does not return anything, so you have to change that. 3 easy steps. Start in i2c_driver.h and change the prototype of the function like this: INLINE int mssp1_waitForEvent(uint16_t*); Change the mssp code to this: inline int mssp1_waitForEvent(uint16_t *timeout) { uint16_t to = 50000; // This will cause a 50ms timeout on I2C if(PIR3bits.SSP1IF == 0) { while(to--) { if(PIR3bits.SSP1IF) break; __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms } if (to == 0) { return -1; } } return 0; } And then lastly catch the error in i2c_master.c inline void i2c_poller(void) { while(i2c_status.busy) { if (mssp1_waitForEvent(NULL) == -1) { i2c_status.state = I2C_ADDRESS_NACK; // State Override for timeout case } i2c_ISR(); } } So what this does is pass up -1 when there is a timeout, which will then advance the state machine (which happens in i2c_ISR() ) based on the status. Now as I said I just used the Address NAK behavior there, when you have no slave connected you should see an address NAK (I should actually check why you are not getting one), but in this case we are saying a timeout requires the same behavior which should work ok. If it does not we may want to add a state for timeout as I described before. But let's try the timeout using the I2C_ADDRESS_NACK method first for your board. If this does not work I will crack out a board and run it on the hardware to see exactly what is happening.
  13. It is just the other layer, it has mssp at the bottom, i2c_driver and master and simple on top of that. I just did not pay close enough attention there, I did not check if it would compile, let me check it out tomorrow and I can help you to get that to build.
  14. Last time that happened to me was yesterday!
  15. If I had a dollar for every time MPLAB puts a modal dialog box under the main window so everything is "frozen". I would retire. Put your rant below:
  16. It looks like you switched microcontrollers. I noticed the generated code was for an MSSP and now it is for an I2C peripheral. Which MCU are you using with the errors above?
  17. So this is where my limited grasp of C stops me. Using that last bit of code you posted, I see what you are doing, but I get an error upon compiling. mcc_generated_files/i2c1_driver.c:100:1: error: (192) undefined identifier "i2c_status" mcc_generated_files/i2c1_driver.c:100:18: error: (196) struct/union required mcc_generated_files/i2c1_driver.c:100:20: error: (192) undefined identifier "I2C_ADDRESS_NACK" I'm gonna crack my C textbook open and see if I can't figure this out.
  18. If you look at the MPLAB Code Configurator timeout driver you will find that each "timer" that you want to create has a structure similar to this: struct timeout { int (*callback)(void *argument); struct timeout *next; // other housekeeping things } And an API that takes such a structure. void timeout_addTimer(struct timeout *myTimer); struct timeout myTimer = {myCallback,0}; void main(void) { timeout_addTimer(&myTimer); } Note: the function names are similar in purpose to MCC but not identical. The function addTimer will accept the completely allocated timer, finish initializing the "private" data and insert this into the list of timers. The memory is "owned" and "provided" by the user but the driver will manage it. Of course this driver will also work with a heap if that is desired. void main(void) { struct timeout *myTimer = malloc(sizeof(struct timeout)); myTimer.callback = myCallback; timeout_addTimer(myTimer); // after some time has passed free(myTimer); } In both cases the application is 100% responsible for the memory and can do the most suitable thing for the circumstances. Just to be "complete" here are two other options: The stack: void main(void) { struct timeout myTimer = {myCallback,0}; timeout_addTimer(&myTimer); while(1) { // do my application } } // This is safe because myTimer will never go out of scope. // However, if you use this technique in a function that will exit, // the memory will go out of scope on the stack and there will be // a difficult to debug crash when the timer expires. Static void foo(void) { static struct timeout myTimer = {myCallback,0}; timeout_addTimer(&myTimer); } Now the timer will be in scope because the variable is static. Of course you may have a new problem that addTimer could be called with this timer already in the timer list. That may break the linked list if the list is not pre-scanned for preexisting timers. In EVERY case I do prefer static allocation that is the responsibility of the application. That assures the following: Code fails during compile/link time so it is easier to debug. Memory usage appears in the data segment so it is tracked by ALL compilers as RAM usage giving an accurate measure of RAM. The driver is scale able because it NEVER needs internally allocated memory nor does it need heap access. That said, I just started using the ESP32 and it's memory is fragmented. It has a block of RAM that is used for static variables, and the data segment. And it has a different larger block of RAM used for the heap. If you don't use the heap, you will have very little memory. Good heap hygiene should be a future topic for discussion.
  19. Ok, I am back up and running and I see that with that you will end up stuck in the next loop. You can force it to break out of the loop by simulating an error. The correct error for your case, and correct behavior in most cases when you get a timeout, would be to perform the AddressNAK behavior. You can trigger that by doing this: inline void mssp1_waitForEvent(uint16_t *timeout) { uint16_t to = 50000; // This will cause a 50ms timeout on I2C if(PIR3bits.SSP1IF == 0) { while(to--) { if(PIR3bits.SSP1IF) break; __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms } if (to == 0) { i2c_status.state = I2C_ADDRESS_NACK; // State Override for timeout case } } } If you want different behavior in the case of a timeout than you have for an address NAK you can always add an event to the state table called stateHandlerFunction, and at this location set the state to the new entry, and then copy the do_I2C_DO_ADDRESS_NACK() function and change the behavior in that to what you want to do different for a timeout. You may e.g. set the error to I2C_TIMEOUT which you could add to i2c_error_t and you probably do not want to do the do_I2C_SEND_RESTART. All of that is of course a more substantial change, but I could walk you through this if you want.
  20. You can use statically linked memory (like a global array of items) to allocate, or you can allocate it on the stack by creating the variable in the function where it is being used. In the case where you statically allocate it the case where you run out of memory will cause the code not to compile, which means of course when it does compile you are guaranteed that you can never fail in this way. If you allocate from any pool (or heap) you will always have to be excessively careful as things like fragmentation can easily catch you. FreeRTOS e.g. has a safe heap implementation which has malloc but no free, that allows you to call malloc during init to initialize all the memory you need, and get determinisitc failure if you do not have enough memory which is easy to debug. If you have a free then making it safe is substantially more difficult because you will fail at the point of maximum memory pressure and this will likely be a race condition which may have low probability of hapening during your testing. The best course of action is to design the system in such a way that it does not compile when you do not have sufficient resources, that way there is no guessing game. Proper error checking only gets you so far. I have seen a case where the init function of the UART (which was the only user interface) failed to allocate memory (linker settings were wrong of course), but the point is that error checking would not have helped much in that case. I have also seen similar cases where there is no way to recover especially in bootloaders.
  21. That delay is central to solving your problem. It is really part of some unfinished timeout code at the top of that function which you thankfully included into your question! The idea was that you can use the concept shown in the comments to produce a timeout in the driver. If you do not expect any timeouts you can safely remove the delay, it simply makes the timeout be a multiple of the delay number, so if we delay 100us, and we set timeout to 100 (the default) then we will get a 10ms timeout. What I would suggest is that you complete the timeout implementation as follows: First you need to set the timeout variable to a fixed value, and then we will reduce the unit delay to 1us (you could remove it altogether and the timeout will be 50,000 loop cycles. If this is too short you can always increase the variable to 32bit and increase the count or add a couple of NOP's to the function. It should look like this when you are done: inline void mssp1_waitForEvent(uint16_t *timeout) { uint16_t to = 50000; // This will cause a 50ms timeout on I2C if(PIR3bits.SSP1IF == 0) { while(to--) { if(PIR3bits.SSP1IF) break; __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms } } } MPLAB-X has died on me today, I am re-installing and will check out if this is sufficient to fix your problem as soon as I have it back up and running, in the meantime you could try the above.
  22. Hello All, I am working on a project (in MPLAB v5.10) with a PIC18F27K40 (PIC18 library v1.77.0) and I'm using the MCC (v3.85.1) generated I2C drivers, I2CSIMPLE from the Foundation Services. I can read from and write successfully to devices on the I2C bus. The problem comes when I try to communicate with a device that's not on the bus, the micro goes into an endless loop waiting for i2c_status to not be busy. My knowledge of programming in C is about 6 on a scale of 10, and for programming for embedded purposes, about 5 out of 10. I would like to have it so that I can check if a specific device is present on the I2C bus, and also be able to recover from errors on the bus without it going into a loop indefinitely. This I2C driver is pretty complex, and I am having difficulties wrapping my head around it. How would I make it so that the driver just returns an error or something I can check for status, rather than loop endlessly until the operation completes, which it never will? I have not edited any of the MCC generated code. This includes leaving the IRQ enable line commented in the i2c_master.c file, so instead it polls instead of using an interrupt to check if the i2c operation has completed. // uncomment the IRQ enable for an interrupt driven driver. // mssp1_enableIRQ(); Following is an example of how I am calling the i2c driver. i2c_write1ByteRegister(address, 0x0D, 0x07); // GPPUB pull-ups on pins 0-2 I am attempting to initialize a port extender, MCP23018, specifically enabling some pull-up resistors. I would like to issue this command, and if the extender is not present, then the micro will perform some tasks differently. With the port extender present the write operation works as expected and everything is fine. Of course the problem is when the extender is NOT on the bus to acknowledge. I have another question as well. This driver seems to operate a little slow. When watching the bus with a logic analyzer I noticed a rather long pause between bytes. I went looking through the i2c driver and in i2c1_driver.c I found the following code which I suspect is the cause. inline void mssp1_waitForEvent(uint16_t *timeout) { // uint16_t to = (timeout!=NULL)?*timeout:100; // to <<= 8; if(PIR3bits.SSP1IF == 0) { while(1)// to--) { if(PIR3bits.SSP1IF) break; __delay_us(100); } } } What is the purpose of the 100 us delay in the while loop? Reducing or eliminating the delay results in reducing or removing the pause between byte transactions, but I don't know enough to know how else this edit will effect the driver. Also, what is the commented out code at the top of the function used for? Is this part of the infinite loop problem I mentioned above? -- James Burkart
  23. With very good reason in embedded systems. Where is the memory allocated from, if not the heap? What happens when there is no more memory available? You will be back to bugs like, "when I am using the app I no longer get notification e-mails" or much worse. Some RTOSs expect you to create memory pools at compile time that are handled much better than simple malloc()/free() but you are still in the same boat of having fixed, limited resources from which to allocate. The real answer is to have proper error checking and give the user a meaningful hint so that they know why they are no longer seeing the expected behaviour.
  24. All too often I see programmers stumped trying to lay out the folder structure for their embedded C project. My best advice is that folder structure is not the real problem. it is just one symptom of dependency problems. If we fix the underlying dependencies a pragmatic folder structure for your project will probably be obvious due to the design being sound. In this blog I am going to first look briefly at Modularity in general, and then explore some program folder structures I see often, exploring if and why they smell. On Modularity in general Writing modular code is not nearly as easy as it sounds. Trying it out for real we quickly discover that simply distributing bits of code across a number of files does not solve much of our problems. This is because modularity is about Architecture and Design and, as such, there is a lot more to it. To determine if we did a good job we need to first look at WHY. WHY exactly do we desire the code to be modular, or to be more specific - what exactly are we trying to achieve by making the code modular? A lot can be said about modularity but to me, my goals are usually as follows: Reduce working set complexity through divide and conquer. Avoid duplication by re-using code in multiple projects (mobility). Adam Smith-like division of labor. When code is broken down into team-sized modules we can construct and maintain it more efficiently. Teams can have areas of specialization and everyone does not have to understand the entire problem in order to contribute. In engineering, functional decomposition is a process of breaking a complex system into a number of smaller subsystems with clear distinguishable functions (responsibilities). The purpose of doing this is to apply the divide and conquer strategy to a complex problem. This is also often called Separation of Concerns. If we keep that in mind we can test for modularity during code review by using a couple of simple core concepts. Separation: Is the boundary of every module clearly distinguishable? This requires every module to be in a single file, or else - if it spans multiple files - a single folder which encapsulates the contents of the module into a single entity. Independent and Interchangeable: This implies that we can also use the module in another program with ease, something Robert C Martin calls Mobility. A good test is to imagine how you would manage the code using version control systems if the module you are evaluating had to reside in a different repository, have its own version number and its own independent documentation. Individually testable: If a module is truly independent it can be used by itself in a test program without bringing a string of other modules along. Testing of the module should follow the Open-Closed principle which means that we can create our tests without modifying the module itself in any way. Reduction in working set Complexity: If the division is not making the code easier to understand it is not effective. This means that modules should perform abstraction - hiding as much of the complexity inside the module and exposing a simplified interface one layer of abstraction above the module function. Software Architecture is in the end all about Abstraction and Encapsulation, which means that making your code modular is all about Architecture. By dividing your project into a number of smaller, more manageable problems, you can solve each of these individually. We should be able to give each of these to a different autonomous team that has its own release schedule, it's own code repository and it's own version number. Exploring some program file structures Now that we have established some ground rules for testing for modularity, let's look at some examples and see if we can figure out which ones are no good and which ones can work based on what we discussed above. Example 1: The Monolith I would hope that we can all agree that this fails the modularity test on all counts. If you have a single file like this there really is only one way to re-use any of the code, and that is to copy and paste it into your other project. For a couple of lines this could still work, but normally we want to avoid duplicating code in multiple projects as this means we have to maintain it in multiple places and if a bug was found in one copy there would be no way to tell how many times the code has been copied and where else we would have to go fix things. I think what contributes to the problem here is that little example projects or demo projects (think about that hello world application) often use this minimalistic structure in the interest of simplifying it down to the bare minimum. This makes sense if we want to really focus on a very specific concept as an example, but it sets a very poor example of how real projects should be structured. Example 2: The includible main In this project, main.c grew to the point where the decision was made to split it into multiple files, but the code was never redesigned, so the modules still have dependencies back to main. That is usually when we see questions like this on Stack Overflow. Of course main.c cannot call into module.c without including module.h, and the module is really the only candidate for including main.h, which means that you have what we call a circular dependency. This mutual dependency indicates that we do not actually have 2 modules at all. Instead, we have one module which has been packaged into 2 different files. Your program should depend on the modules it uses, it does not make sense for any of these modules to have a reverse dependency back to your program, and as such it does not make any sense to have something like main.h. Instead, just place anything you are tempted to place in main.h at the top of main.c instead! If you do have definitions or types that you think can be used by more than one module then make this into a proper module, give it a proper name and let anything which uses this include this module as a proper dependency. Always remember that header files are the public interfaces into your C translation unit. Any good Object Oriented programming book will advise you to make as little as possible public in your class. You should never expose the insides of your module publically if it does not form part of the public interface for the class. If your definitions, types or declarations are intended for internal use only they should not be in your public header file, placing them at the top of your C file most likely the best. A good example is device configuration bits. I like to place my configuration bit definitions in a file by itself called device_config.h, which contains only configuration bit settings for my project. This module is only used by main, but it is not called main.h. Instead, it has a single responsibility which is easy to deduce from the name of the file. To keep it single responsibility I will never put other things like global defines or types in this file. It is only for setting up the processor config bits and if I do another project where the settings should be the same (e.g. the bootloader for the project) then it is easy for me to re-use this single file. In a typical project, you will want to have an application that depends on a number of libraries, something like this. Importantly we can describe the program as an application that uses WiFi, TempSensors, and TLS. There should not be any direct dependencies between modules. Any dependencies between modules should be classified as configuration which is injected by the application, and the code that ties all of this together should be part of the application, not the modules. It is important that we adhere to the Open-Closed principle here. We cannot inject dependencies by modifying the code in the libraries/modules that we use, it has to be done by changing the application. The moment we change the libraries to do this we have changed the library in an application-specific way and we will pay the price for that when we try to re-use it. It is always critical that the dependencies here run only in one direction, and that you can find all the code that makes up each module on your diagram in a single file or in a folder by itself to enable you to deal with the module as a whole. Example 3: The Aggregate or generic header file Projects often use an aggregate header file called something like "includes.h". This quickly leads to the pattern where every module depends on every other and is also known as Spaghetti Code. It becomes obvious if you look at the include graph or when you try and re-use a module in your project by itself for e.g. a test. When any header file is changed you have to re-test every module now. This fails the test of having clearly distinguishable boundaries and clear and obvious dependencies between modules. In MCC there is a good (or should I say bad?) example of such an aggregate header file called mcc.h. I created a minimal project using MCC for the PIC16F18877 and only added the Accel3 click to the project as a working example for this case. The include graph generated using Doxygen looks as follows. There is no indication from this graph that the Accelerometer is the one using the I2C driver, and although main never calls to I2C itself it does look like that dependency exists. The noble intention here is of course to define a single external interface for MCC generated code, but it ends up tying all of the MCC code together into a single monolithic thing. This means my application does not depend on the Accelerometer, it now depends instead on a single monolithic thing called "everything inside of MCC", and as MCC grows this will become more and more painful to manage. If you remove the aggregate header then main no longer includes everything and the kitchen sink, and the include graph reduces to something much more useful as follows: This works better because now the abstractions are being used to simplify things effectively, and the dependency of the sensor on I2C is hidden from the application level. This means we could change the sensor from I2C to SPI without having any impact on the next layer up. Another version of this anti-pattern is called "One big Header File", where instead of making one header that includes all the others, we just place all the contents of all those headers into a single global file. This file is often called "common.h" or "defs.h" or "global.h" or something similar. Ward Cunningham has a good comprehensive list of the problems caused by this practice on his wiki. Example 4: The shared include folder This is a great example of Cargo Culting something that sometimes works in a library, and applying it everywhere without understanding the consequences. The mistake here is to divide the project into sources and headers instead of dividing it into modules. Sources and headers are hopefully not the division that comes to mind when I ask you to divide code into modules! In the context of a library, where the intention is very much to have an external interface (include) separated from its internal implementation (src), this segregation can make sense, but your program is not a library. When you look at this structure you should ask how would this work in the following typical scenarios: If one of the libraries grows enough that we need to split it into multiple files? How will you now know which headers and/source belong to which library? If two libraries end up with identically named files? Typical examples of collisions are types.h, config.h, hal.h, callbacks.h or interface.h. If I have to update a library to a later version, how will I know which files to replace if they are all mixed into the same folder? How do I know which files are part of my project, and as such, I should maintain them locally, as opposed to which files are part of a library and should be maintained at the library project location which is used in many projects? This structure is bad because it breaks the core architectural principles of cohesion and encapsulation which dictates that we keep related things together, and encapsulate logical or functional groupings into clearly identifiable entities. If you do not get this right it leads to library files being copied into every project, and that means multiple copies of the same file in revision control. You also end up with files that have nothing to do with each other grouped together in the same folder. Example 5: A better way On the other hand, if you focus on cohesion and encapsulation you should end up with something more like this I am not saying this is the one true way to structure your project, but with this arrangement, we can get the libraries from revision control and simply replace an entire folder when we do. It is also obvious which files are part of each library, and which ones belong to my project. We can see at a glance that this project has it's own code and depends on 3 libraries. The structure embodies information about the project which helps us manage it, and this information is not duplicated requiring us to keep data from different places in sync. We can now include these libraries into this, or any other project, by simply telling Git to fetch the desired version of each of these folders from its own repository. This makes it easy to update the version of any particular library, and name collisions between libraries are no longer an issue. Additionally, as the library grows it will be easy to distinguish in my code which library I have a dependency on, and exactly which types.h file I am referring to when I refer to the header files as follows. Conclusion Many different project directory structures could work for your project. We are in no way saying that this is "the one true structure". What we are saying is that when the time comes to commit your project to a structure, do remember the pros and cons of each of these examples we discussed. That way you will at least know the coming consequences of your decisions before you are committed to them. Robert C. Martin, aka Uncle Bob, wrote a great article back in 2000 describing the SOLID architectural principles. SOLID is focussed on managing dependencies between software modules. Following these principles will help create an architecture that manages the dependencies between modules well. A SOLID design will naturally translate into a manageable folder structure for your embedded C project.
  25. Sometimes I get the sad impression that embedded FW engineers only understand 1 data container, the array. The array is a powerful thing and there are many good reasons to use it but it also has some serious problems. For instance, a number of TCP/IP libraries use arrays to hold a static list of sockets. When you wish to create a new socket the library takes one of the unused sockets from the array and returns a reference (a pointer or index) so the socket can be used as a parameter for the rest of the API. It turns out that sockets are somewhat heavy things (use lots of memory) so you always want to optimize the system to have the smallest number of sockets necessary. Unfortunately, you must "pick" a reasonable number of sockets early in the development process. If you run out of sockets you must go back and recompile the library to utilize the new socket count. Now there is a dependency that is not obvious, only fails at run time and links the feature count of the product with the underlying library. You will see bugs like, "when I am using the app I no longer get notification e-mails". It turns out that this problem can be easily solved with a dynamic container. i.e. one that grows at runtime as you need it to. A brute force method would perhaps be to rely upon the heap to reallocate the array at runtime and simply give the library a pointer to an array. That will work but it inserts a heavy copy operation and the library has to be paused while the old array is migrated to the new array. I propose that you should consider a Linked List. I get a number of concerns from other engineers when I have made this suggestion so just hang tight just one moment. Concerns Allocating the memory requires the heap and my application cannot do that. Traversing the list is complicated and requires recursion. We cannot afford the stack space. A linked list library is a lot of code to solve this problem when a simple array can manage it. The linking pointers use more memory. If you have a new concern, post it below. I will talk about these first. Concern #1, Memory allocation I would argue that a heap is NOT required for a linked list. It is simply the way computer science often teaches the topic. Allocate a block of memory for the data. place the data in the block of memory. Data is often supplied as function parameters. insert the block into the list in the correct place. Computer science courses often teach linked lists and sorting algorithms at the same time so this process forms a powerful association. However, what if the process worked a little differently.j Library Code -> Define a suitable data structure for the user data. Include a pointer for the linked list. User Code -> Create a static instance of the library data structure. Fill it with data. User Code -> Pass a reference to the data structure to the library. Library Code -> insert the data structure into the linked list. If you follow this pattern, the user code can have as many sockets or timers or other widgets as it has memory for. The library will manage the list and operate on the elements. When you delete an element you are simply telling the library to forget but the memory is always owned by the user application. That fixes the data count dependency of the array. Concern #2, Traversing the list is complex and recursive. First, Recursion is always a choice. Just avoid it if that is a rule of your system. Every recursive algorithm can be converted to a loop. .Second, Traversing the list is not much different than an array. The pointer data type is larger so it does take a little longer. struct object_data { int mass; struct object_data *nextObject; }; int findTheMassOfTheObjects(struct object_data *objectList) { thisObject = objectList; while(thisObject) { totalMass += thisObject->mass; thisObject = thisObject->nextObject; } printf("The mass of all the objects is %d grams\n", totalMass); return totalMass; } So here is a quick example. It does have the potential of running across memory if the last object in the list does NOT point at NULL. So that is a potential pitfall. Concern #3, A linked list library is a lot of code Yes it is. Don't do that. A generic library can be done and is a great academic exercise but most of the time the additional next pointers and a few functions to insert and remove objects are sufficient. The "library" should be a collection of code snippets that your developers can copy and paste into the code. This will provide reuse but break the dependency on a common library allowing data types to change, or modifications to be made. Concern #4, A linked list will use more memory It is true that the linked list adds a pointer element to the container data structure. However, this additional memory is probably much smaller than the "just in case" additional memory of unused array elements. It is probably also MUCH better than going back and recompiling an underlying library late in the program and adding a lot more memory so the last bug will not happen again. A little history The linked list was invented by Allen Newell, Cliff Shaw and Herbert Simon. These men were developing IPL (Information Processing Language) and decided that lists were the most suitable solution for containers for IPL. They were eventually awarded a Turing Award for making basic contributions to AI, Psychology of Human Cognition and list processing. Interestingly IPL was developed for a computer called JOHNIAC which had a grand total of 16 kbytes of RAM. Even with only 16KB IPL was very successful and linked lists were determined to be the most suitable design for that problem set. Most of our modern microcontrollers have many times that memory and we are falling back on arrays more and more often. If you are going to insist on an array where a linked list is a better choice, you can rest easy knowing that CACHE memory works MUCH better with arrays simply because you can guarantee that all the data is in close proximity so the entire array is likely living in the cache. Good Luck P.S. - The timeout driver and the TCP library from Microchip both run on 8-bit machines with less than 4KB of RAM and they both use linked lists. Check out the code in MCC for details.
  26. One important rule for good software is Do not Repeat Yourself. This rule is often referred to under the acronym DRY. Most of the time, your code can be made DRY by refactoring repeating blocks of code into a function. This is a good practice but it can lead to a lot of small functions. You undoubtedly will keep your API's tidy by making these functions static and keeping them close to where they are needed but I have recently been working on a lot of C++ code and I have a new tool in my programming toolbox that I would like to share with you. The LAMBDA function. Essentially a lambda is a function that you define inside of another function. This lambda function has function scope so it can only be used in the function that defines it. This can be very useful and will do two things to help keep your code maintainable. It keeps your code readable by forcing you to define a function close to where it is needed. It encourages you to keep the function short & sweet because you will not be tempted to make it a "general purpose solution". Here is an example. I was tasked to implement a serializer where data would be sent on the serial port. This was a binary protocol and it included a special character for start of frame (SOF) and end of frame (EOF). Because the SOF and EOF characters could appear in the actual data, there was an additional data link escape (DLC) character sequence that would expand into the SOF, EOF and DLC. For added fun, there is a checksum that is generated BEFORE the character padding. <SOF><DATA><CHECKSUM><EOF> Here is a function that can decode this message. #define MAXIMUM_MESSAGE_SIZE 255 void dataLink::receiveData(char b) { const char SOF = 0x1A; const char EOF = 0x1B; const char DLC = 0x1C; static enum {findSOF, getData} theState; static int messageIndex=0; static char checksum = 0; static char receivedMessage[MAXIMUM_MESSAGE_SIZE]; switch(theState) { case findSOF: if(b == SOF) { theState = getData; messageIndex = 0; checksum = 0; memset(receivedMessage,0,sizeof(receivedMessage)); } break; case getData: { static bool dlc_last = false; if(dlc_last) { dlc_last = false; switch(b) { case 1: receivedMessage[messageIndex++] = 0x1A; checksum += 0x1A; break; case 2: receivedMessage[messageIndex++] = 0x1B; checksum += 0x1B; break; case 3: receivedMessage[messageIndex++] = 0x1C; checksum += 0x1C; break; } } else { switch(b) { case EOF: theState = findSOF; if(checksum == 0) { //********************* // Do something with the new message //********************* } break; case DLC: dlc_last = true; break; default: receivedMessage[messageIndex++] = b; checksum += b; break; } } break; } } } This function receives a byte and using a few states, creates a checksum validated array of decoded bytes representing the message. I will not explain each line as the details of this function are really not very important. As my code reviewer you should instantly notice that there are 4 sections of nearly identical code that are repeated. In other words, this is not DRY. My first inclination would be to attempt to reorder the decisions so the update of the message array and checksum was done once. This method of course works quite well in this case but I wanted a simple contrived example to show off the lambda function. #define MAXIMUM_MESSAGE_SIZE 255 void dataLink::receiveData(char b) { const char SOF = 0x1A; const char EOF = 0x1B; const char DLC = 0x1C; static enum {findSOF, getData} theState; static int messageIndex=0; static char checksum = 0; static char receivedMessage[MAXIMUM_MESSAGE_SIZE]; auto output_byte = [&](char b) { receivedMessage[messageIndex++] = b; checksum += b; }; switch(theState) { case findSOF: if(b == SOF) { theState = getData; messageIndex = 0; checksum = 0; memset(receivedMessage,0,sizeof(receivedMessage)); } break; case getData: { static bool dlc_last = false; switch(b) { case EOF: theState = findSOF; if(checksum == 0) { //********************* // Do something with the new message //********************* } break; case DLC: dlc_last = true; break; default: if(dlc_last) { dlc_last = false; switch(b) case 1: output_byte(0x1A); break; case 2: output_byte(0x1B); break; case 3: output_byte(0x1C); break; } else { output_byte(b); } break; } break; } } } Now you can see that I moved the work of inserting the byte into the array and updating the checksum into a function called output_byte. This function is defined inside the receiveData function. The syntax has a square bracket followed by parenthesis. The Ampersand inside the brackets indicates that the function has access to all the variables inside receiveData. This makes the function very simple and easy to verify by inspection. Of course you could have made the output_byte function a private member function of the class. But you would have needed to add the checksum and the index variables to the class as well. That increases the complexity of the class. By using the lambda, the function can be made DRY, and readable, and the function does not leak information or variables into any other code. This makes the function much simpler to maintain or potentially refactor in the future. BTW, I tested this function by building the project in the Arduino environment on a SAMD21. The actual Arduino IDE is very limited but when you use VIsual Studio Code and PlatformIO you get full debug on an ATMEL ICE with the Arduino frameworks. This makes developing interesting applications VERY fast. lambda_demo.zip
  27. Some advice for Microchip: If this was my product I would stop selling development kits with A1 or A3 silicon to customers. I2C is widely used and it will create a really bad impression of the product's reliability if customers were to evaluate it with defective silicon. And please fix the Errata, your workaround for I2C Issue 1 does not work as advertized !
  1. Load more activity
 


×
×
  • Create New...