DynamicPerception / OMLibraries

OpenMoCo Libraries for AVR
GNU General Public License v3.0
35 stars 28 forks source link

Porting OMMenuMgr to chipKIT... #4

Closed cacycleworks closed 11 years ago

cacycleworks commented 11 years ago

Hi,

Is there any chance I could get help with moving the static menu definitions into the Class? I'm trying to get this code working on my chipKIT platform. The structures and PROGMEM pointer stuff is difficult for me to get a grasp on.

I made a post about it with code Zip on chipKIT here: http://chipkit.org/forum/viewtopic.php?p=9714#p9714

I'm pretty sure I'm missing something easy-ish but ugh, all the pointer stars. :(

thisdroneeatspeople commented 11 years ago

Ok, it looks like chipkit doesn't support progmem (I don't use PIC32's or chipkit myself, so I can only address the obvious things I can quickly google here =) - so, you'll need to first get rid of the PROGMEM strings from #define's near the top of the header file. Then, to minimize other code modified, simply re-define prog_char:

#define prog_char char*

Next, you'll need to make some changes to the code its self, these aren't hard, and aren't hard to make. Look for pgm_* in the CPP file. These functions copy data from flash space to SRAM. You don't need to do that, as your processor has only one memory space to access. So, get rid of it, for example:

uint8_t childCount     = pgm_read_byte(&(p_item->targetCount));

Becomes:

uint8_t childCount     = p_item->targetCount;

I do not understand the last paragraph of your post on that forum, if you want to use additional memory and create object instances for every menu item in your menu, you are welcome to re-write each struct as a class (which makes little sense, since they don't actually have any functionality other than to express a formal definition of the data, hence why they are structs and not classes). There should be no need to change anything from a struct to make it run on another platform, this is standard C++.

To clarify: the use of #defines and structs are not to "make it compile," the #defines were created to reduce the hand-written code, since we have to deal with PROGMEM in smaller uC's, and the structs are the proper definition of data types which represent the components of the menu. For an example of a larger menu in action, see: https://github.com/DynamicPerception/MX3_LightMotion/blob/master/Firmware/MX3_LightMotion/MX3_UI.h#L151

cacycleworks commented 11 years ago

Thanks for your reply - very much appreciated! I'll give your suggestions a go on a fresh copy of the library. When I went through it, something is off in my version, as the text strings aren't making it. Nice, I'll have this on my mind to help me leave the Labor Day party a little early tonight!

Thanks, Chris

California Cycleworks, Inc www.ca-cycleworks.com P: 619/501-2466 F: 619/569-1917

http://www.facebook.com/californiacycleworks http://twitter.com/cacycleworks http://www.linkedin.com/company/2226216?trk=tyah https://plus.google.com/u/0/113578285164626977449/posts http://ca-cycleworks.com/videos http://ducatitech.com http://www.ducatimonsterforum.org/index.php?www;board=75 http://www.ducati.ms/forums/206-california-cycleworks/

On Mon, Sep 2, 2013 at 6:18 AM, C. A. Church notifications@github.comwrote:

Ok, it looks like chipkit doesn't support progmem (I don't use PIC32's or chipkit myself, so I can only address the obvious things I can quickly google here =) - so, you'll need to first get rid of the PROGMEM strings from #define's near the top of the header file. Then, to minimize other code modified, simply re-define prog_char:

define prog_char char*

Next, you'll need to make some changes to the code its self, these aren't hard, and aren't hard to make. Look for pgm_* in the CPP file. These functions copy data from flash space to SRAM. You don't need to do that, as your processor has only one memory space to access. So, get rid of it, for example:

uint8_t childCount = pgm_read_byte(&(p_item->targetCount));

Becomes:

uint8_t childCount = p_item->targetCount;

I do not understand the last paragraph of your post on that forum, if you want to use additional memory and create object instances for every menu item in your menu, you are welcome to re-write each struct as a class (which makes little sense, since they don't actually have any functionality other than to express a formal definition of the data, hence why they are structs and not classes). There should be no need to change anything from a struct to make it run on another platform, this is standard C++.

To clarify: the use of #defines and structs are not to "make it compile," the #defines were created to reduce the hand-written code, since we have to deal with PROGMEM in smaller uC's, and the structs are the proper definition of data types which represent the components of the menu. For an example of a larger menu in action, see: https://github.com/DynamicPerception/MX3_LightMotion/blob/master/Firmware/MX3_LightMotion/MX3_UI.h#L151

— Reply to this email directly or view it on GitHubhttps://github.com/DynamicPerception/OMLibraries/issues/4#issuecomment-23659795 .

cacycleworks commented 11 years ago

Hi, yeah, same result with performing your suggested edits. BTW, added this define: #define memcpy_P memcpy and had to change #include "Arduino.h" to #include "WProgram.h". Additionally, #include <avr/pgmspace.h> needs to be commented out, as chipKIT's IDE sees that and refuses to go on.

I'm fairly sure that the problem stems from trying to get the address of the text label itself to send to the buffer. Callbacks to uiDraw and uiClear seem otherwise ok. Below is the resulting code from the edits. Your already commented out line, when compiled with pgm_read_word removed resulted in an invalid cast of non-class void * error.

Also tried the code snippet below the function. I have seen times when Processing IDE can't handle multiple actions on a line of code (more than likely are macros buried in the original IDE library?)

void OMMenuMgr::_displayList(OMMenuItem* p_item, uint8_t p_target) {
    uint8_t childCount     = p_item->targetCount;
    childCount--;
    //  OMMenuItem** items = reinterpret_cast<OMMenuItem**>(reinterpret_cast<void*>(pgm_read_word(&(p_item->target))));
    //  m_curSel = reinterpret_cast<OMMenuItem*>(pgm_read_word(&(items[p_target])));
    OMMenuItem** items = reinterpret_cast<OMMenuItem**>(reinterpret_cast<void*>(&(p_item->target)));
    m_curSel = reinterpret_cast<OMMenuItem*>(&(items[p_target]));
    // loop through display rows
    for(byte i = 0; i < OM_MENU_ROWS; i++ ) {
        // flush buffer
        memset(m_dispBuf, ' ', sizeof(char) * sizeof(m_dispBuf));
        int startX = 0;
        // display cursor on first row
        if( i == 0 ) {
            _display((char*) OM_MENU_CURSOR, i, 0, sizeof(char) * sizeof(OM_MENU_CURSOR) - 1);
            startX += ( sizeof(char) * sizeof(OM_MENU_CURSOR) - 1 );
        } else {
            _display((char*) OM_MENU_NOCURSOR, i, 0, sizeof(char) * sizeof(OM_MENU_NOCURSOR) - 1);
            startX += ( sizeof(char) * sizeof(OM_MENU_NOCURSOR) - 1 );
        }
        // if there's actually an item here, copy it to the display buffer
        if( childCount >= p_target + i ) {
            //  Already commented out: OMMenuItem* item = reinterpret_cast<OMMenuItem*>( pgm_read_word(p_item->target.items[p_target + i]) );
            //  OMMenuItem* item = reinterpret_cast<OMMenuItem*>(pgm_read_word(&(items[p_target + i])));
            OMMenuItem* item = reinterpret_cast<OMMenuItem*>(&(items[p_target + i]));
            memcpy_P(m_dispBuf, &( item->label ), sizeof(char) * sizeof(m_dispBuf) );
        }
        _display(m_dispBuf, i, startX, OM_MENU_COLS);
    } 
}
//          void * anaddr = &(items[p_target + i]);
//          OMMenuItem* item = reinterpret_cast<OMMenuItem*>(anaddr);
thisdroneeatspeople commented 11 years ago

Get rid of the taking-address-of operators too - those were necessary for the pgm_* macros. E.g.:

 OMMenuItem* item = reinterpret_cast<OMMenuItem*>(&(items[p_target + i]));

Should be:

 OMMenuItem* item = reinterpret_cast<OMMenuItem*>(items[p_target + i]);

You need to do this everywhere you removed the pgm... code - they were there because we couldn't access progmem space directly, and we needed to ensure that we took the address of the variable instead of trying to read it in the pgm... arguments.

The things stored are already pointers (in this case, a void pointer) and we just have to coerce them into the proper cast (note the c++ style re-casting we do here -- don't try to use c-style implicit casts [e.g.: (OMMenuItem*) foo...] as they will fail in some of the cases, or silently do the wrong thing in other cases).

The reasoning for switching to WProgram.h is because Chipkit is using the much older 0.23 Arduino base -- I'm surprised that it borked on pgmspace.h, I always thought the bad code in there was ifdef'd on platform. But then again, I haven't looked at it in a long time. Actually, you probably don't even have avr/... files at all.

This line here can be simplified, too:

OMMenuItem** items = reinterpret_cast<OMMenuItem**>(reinterpret_cast<void*>(&(p_item->target)));

Could be re-written as:

OMMenuItem** items = reinterpret_cast<OMMenuItem**>(p_item->target);

The reason for the original conversion was because we couldn't cast directly from a uint32_t (what pgm_readword would return) to a dynamic multi-dimensional array. We had to convert to the interim void, but there is no need to now, since pitem->target already is a void.

cacycleworks commented 11 years ago

I finally gave up and implemented something less sophisticated. https://github.com/cacycleworks/chipKIT_ST7735/blob/master/Libraries/ST7735/examples/simpleMenu/simpleMenu.ino

I spent a week on this and couldn't find an OOP example of how to make a generic menu framework. I at least got the above to compile and actually work. Thanks for your time helping!

thisdroneeatspeople commented 11 years ago

No problem! Glad you were able to come to a solution that works for you!