Jump to content
 

Orunmila

Member
  • Content Count

    221
  • Joined

  • Last visited

  • Days Won

    28

Posts posted by Orunmila


  1. OpenOCD is not really CPU specific. It supports debugging via GDB. It allows you to set breakpoints and single step the code. This is all the stuff you had to implement already. If you have GDB support via OpenOCD for the programmer you will be able to use it with IDE's such as VSCode, Eclipse etc. to debug the device. 

     


  2. I tested it and from v1.80.0 of the PIC10/12/16 library onwards this seems to be broken. If you open the "Versions" tab and find the previous version it still works fine. You can switch to the old version in the IDE. It looks like this on my machine.

    image.png

     

    Right-click on the 1.78.1 version and select Mark library for load and then press the load button at the top next to where it says "Versions" it will appear. After doing this I bet it will work just fine.

    It builds. I will attach the project for you also.

     


  3. I had a similar choice on the CO2 valve for my fishtank, but if my sw had any glitch the tank would be carbonated like soda and everything would die from the ph drop, so I decided to leave the mechanical regulator in the line for a backup if the electronics fail for some reason. You never know what happens when the power goes out or a battery dies at the wrong time...


  4. Provided nothing went wrong in your analysis that bit being set means that somewhere something did a software reset, you just have to figure out where this happened. N9WXU gave some possible causes of the RESET instruction being used, but it could really be anywhere. I have even seen this happen with a bad function pointer jumping to a location which contained const data.

    As a last resort you can search through the program memory for the RESET instruction (you can do this in MPLAB using the memory views) and set a breakpoint at every location containing the reset instruction. That way you should be able to catch it in the debugger and figure out where the reset is coming from.

    • Helpful 1

  5. We just started a new blog which aims to create some benchmarks for comparing performance of small microcontrollers. This is something that we always needed to do ourselves as part of processor selection, so we decided to share our experiments and results here for everybody to use.

     

     

     


  6. On 11/26/2019 at 7:15 AM, mehmetozdemir said:

    and "EUSART2_Read"  function seems very complicated

    I think with these drivers it always depends on what you need and if this is a good match for your requirements. If you say that this seems very complicated it sounds like this driver probrably is doing a lot more than you need, so my first question would be "what do you expect instead"?

    I know N9WXU has talked to much of this already and also said it depends on whether you require a ring buffer. What I can offer you is what the thinking was for this implementation.

    For this driver the requirements were:

    1. We have to be able to receive data on interrupt, this ensures we never miss any bytes due to data overrun (next byte received before prior byte is read out).
    2. The application does NOT want to process the data inside of the ISR, so we need to store the data temporarily
    3. We need to be able to store multiple bytes, which means we may have 7 or 8 interrupts before we process the data. This means the application can safely take much longer to process the data before we lose data.

    If you serial port is running 115200 it means one character arrives every 87us. If we have a buffer of 16 bytes it means we only need to service the serial port at least once every 1.4ms to be sure we never lose any data, this extra time can be very important to us, and we can make the buffer bigger to get even more time.

    If this matches your situation you need to do the following.

    In the ISR you need to:

    1. Store the data in a temp buffer

     // use this default receive interrupt handler code
        eusart2RxBuffer[eusart2RxHead++] = RCREG2;

    2. Handle wrapping of the buffer when you reach the end of the array

        if(sizeof(eusart2RxBuffer) <= eusart2RxHead)
        {
            eusart2RxHead = 0;
        }

    3. And keep track of how many bytes we have ready for processing

        eusart2RxCount++;

     

    When the app processes the data you need to:

    1. If you try to read 1 byte before the data is available you need to wait until there is something to read 

        uint8_t readValue  = 0;
        
        while(0 == eusart2RxCount)
        {
        }

    2. Remove one byte from the ring buffer and again handle the wrapping at the end of the array

        readValue = eusart2RxBuffer[eusart2RxTail++];
        if(sizeof(eusart2RxBuffer) <= eusart2RxTail)
        {
            eusart2RxTail = 0;
        }

    3. Reduce the number of bytes in the buffer by one (we need to disable the ISR or a collision can happen here), and then return the retrieved byte

        PIE3bits.RC2IE = 0;
        eusart2RxCount--;
        PIE3bits.RC2IE = 1;
    
        return readValue;

     

    I think this is kind of as simple as you can ever do this without reducing the functionality. 
     

    Of course if you just wanted to without interrupts read the single byte all you need is to return RCREG,  which BTW is what you will get if you uncheck the interrupt checkbox. Also if you do not like all of the ring buffer stuff you can enable interrupts and replace the ISR with the same thing you get when you do not need a buffer, then it is simpler but more likely to lose data.

     

    PS. I did not describe the eusart2RxLastError line as I cannot see all the code for that here and I cannot remember the details about that one line. What it is doing is updating a global (eusart2RxLastError) to indicate if there was any error. From the looks of this code snippet that part of the code may have some bugs in it as the last error is not updated after the byte is read out, but I may just be missing the start of the ISR ...
     

     

     

    • Helpful 1

  7. If there is an explanation of the numbers right there they are not rightfully magical anymore 🙂

    I can embrace that kind of description, as long as you have the discipline to keep the comments and the values in sync.

    But I still prefer describing the register instead of the value, if I want to change the prescaler or the CCP1CON value I now need to reach for the datasheet (although the convenient reference will tell me where!) I would prefer having the entire CCP1CON described here but especially on 32bit machines this can be entirely unpractical and then I will be OK with that scheme.


  8. I do not like the comments describing the magic number's value, I much prefer the comments describing the register here, to help you not having to go to the datasheet.

    The comments should make the static information like the register design clear here so that the reader of the code can understand what the number means by just reading the comment. This is subtly different from the comment claiming the meaning. If you do this the comments will not go out of sync with the code and also help you if you want to change the value to something it is not currently set to.

    Comments should describe WHY we are doing things, these motivations should be design, assumptions or external constraints. 


  9. 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 🙂


  10. 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.

     

     


  11. 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();
        }
    }

     


  12. 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.


  13. 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.

     

     

     

×
×
  • Create New...