feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC #571

MertCanBoz1 · 2023-04-28T11:05:12Z

I changed the AppSwitcher (in "refdes" repository) example to work from the external flash instead of the SD Card. To be able to build the new code, some functions should be changed from here too.

…e (from refdes repository)

Jake-Carter · 2023-05-02T22:41:32Z

Libraries/SDHC/ff13/Source/diskio.c

 /* Definitions of physical drive number for each drive */
 #define DEV_SD      0   /* Example: Map MMC/SD card to physical drive 1 */
 #define SPI_SPEED 10000000

 #ifdef NATIVE_SDHC
-
+#define MX25_EXP_ID 			0x00c2953a


I suggest we move this to mx25.h.

Then, move the validation of the expected ID inside of mscmem_ID, and return a generic boolean.

As a result the check

if (mscmem_ID() == MX25_EXP_ID) { status = 0; } else{ init_done = 0; status = STA_NOINIT | STA_NODISK; }

becomes

if (!mscmem_ID()) { status = STA_NOINIT | STA_NODISK; }

Jake-Carter · 2023-05-02T22:44:03Z

Libraries/SDHC/ff13/Source/diskio.c

+		    DSTATUS status = 0;
+
+
+    if (mscmem_ID() == MX25_EXP_ID) {


Again here, see comment above

Jake-Carter · 2023-05-02T23:13:11Z

Libraries/SDHC/ff13/Source/diskio.c

@@ -31,23 +33,44 @@ static unsigned int init_done = 0;

 /*local vaiables*/
 static uint8_t rtc_en;
-
+uint8_t flash = 0;


I am a little bit confused about using the global flash variable vs #ifdef FLASH in the preprocessor.

Consider the following function:

DSTATUS disk_status ( BYTE pdrv /* Physical drive nmuber to identify the drive */ ) { if(!flash){ DSTATUS status = 0; if (!MXC_SDHC_Card_Inserted()) { init_done = 0; status = STA_NOINIT | STA_NODISK; } return status; } #ifdef FLASH else{ DSTATUS status = 0; if (mscmem_ID() == MX25_EXP_ID) { status = 0; } else{ init_done = 0; status = STA_NOINIT | STA_NODISK; } return status; } #endif return 0; }

Why is the global runtime variable necessary? I don't see any calls to disk_flash in this driver code. Given the compiler definition and the suggestion above we can reduce this function to:

DSTATUS disk_status ( BYTE pdrv /* Physical drive nmuber to identify the drive */ ) { DSTATUS status = 0; #ifndef FLASH init_done = MXC_SDHC_Card_Inserted(); #else init_done = mscmem_ID(); #endif if (!init_done) { status = STA_NOINIT | STA_NODISK; } return status; }

The global external_flash variable is necessary because the new AppSwitcher code will check whether the user uses an SD Card or the external flash (if the SD Card is not inserted, then continue with the external flash). I added a preprocessor because if an application other than AppSwitcher calls the diskio.c, it is not necessary to compile all mscmem functions. The disk_flash() is called from the main.c of the new AppSwitcher, which also exists as a PR in another repository (refdes).

Ok, thank you for clarifying. This situation is a multi-drive system:

(http://elm-chan.org/fsw/ff/doc/appnote.html#port)

The disk_initialize function is already designed to handle this with the pdrv argument. In the image above physical drive 2 is the SD card slot, and physical drive 0 is the external flash.

Following that scheme, the app switcher application code would first attempt to initialize drive 2 (SD). If it fails, then you attempt to initialize drive 0 (Ext Flash).

The diskio.c file in the avr example in ffsample.zip is a decent reference for a multi-drive implementation.

Notice how they are separating the driver layers based on a scheme they have defined for pdrv.

/*-----------------------------------------------------------------------*/ /* Low level disk I/O module glue functions (C)ChaN, 2016 */ /*-----------------------------------------------------------------------*/ /* If a working storage control module is available, it should be */ /* attached to the FatFs via a glue function rather than modifying it. */ /* This is an example of glue functions to attach various exsisting */ /* storage control modules to the FatFs module with a defined API. */ /*-----------------------------------------------------------------------*/ #include "ff.h" /* Obtains integer types for FatFs */ #include "diskio.h" /* FatFs lower layer API */ #ifdef DRV_CFC #include "cfc_avr.h" /* Header file of existing CF control module */ #endif #ifdef DRV_MMC #include "mmc_avr.h" /* Header file of existing SD control module */ #endif // ... /*-----------------------------------------------------------------------*/ /* Inidialize a Drive */ /*-----------------------------------------------------------------------*/ DSTATUS disk_initialize ( BYTE pdrv /* Physical drive nmuber to identify the drive */ ) { switch (pdrv) { #ifdef DRV_CFC case DRV_CFC : return cf_disk_initialize(); #endif #ifdef DRV_MMC case DRV_MMC : return mmc_disk_initialize(); #endif } return STA_NOINIT; }

Jake-Carter · 2023-05-02T23:14:21Z

Libraries/SDHC/ff13/Source/diskio.c

+	    rtc_en = 0;
+#if (FF_FS_NORTC == 0)
+	    //Initialize RTC
+	    if (MXC_RTC->cn & MXC_F_RTC_CN_WE) {


Formatting issue on this block, make sure lines 92 - 101 are indented correctly

Jake-Carter · 2023-05-02T23:19:28Z

Libraries/SDHC/ff13/Source/diskio.c

+
+    status = RES_OK;
+
+#if FF_MAX_SS == 4096


I am a little confused on the driver differences between FF_MAX_SS = 4096 and FF_MAX_SS = 512.

To improve the readability of the code I would suggest adding a comment explaining it, and you can also consolidate inside of the for loop:

for(index = 0; index < count; index++){ #if FF_MAX_SS == 4096 // ... #endif #if FF_MAX_SS == 512 // ... #endif }

I took the base code from the MAXREFDES104 project, and FF_MAX_SS = 4096 part was added to their code. I will remove all FF_MAX_SS = 4096 parts.

Jake-Carter · 2023-05-02T23:21:51Z

Libraries/SDHC/ff13/Source/diskio.c


+
+    status = RES_OK;
+#if FF_MAX_SS == 4096


Same FF_MAX_SS considerations here - please add a comment explaining why it's necessary and consider consolidating the switch statements if possible

Jake-Carter · 2023-05-02T23:27:33Z

Libraries/SDHC/ff13/Source/diskio.h

@@ -40,7 +42,7 @@ DRESULT disk_read (BYTE pdrv, BYTE* buff, DWORD sector, UINT count);
 DRESULT disk_write (BYTE pdrv, const BYTE* buff, DWORD sector, UINT count);
 DRESULT disk_ioctl (BYTE pdrv, BYTE cmd, void* buff);
 DWORD get_fattime(void);
-
+void disk_flash(uint32_t isFlash);


diskio.h modification - adding this function to the API should only be done as an absolute last resort. Additionally, uint32_t is used which goes against the FatFS convention of using its built-in data types.

I am not sure how I can come around to not changing the diskio.h for this code snippet. I will look into it.

See https://github.com/Analog-Devices-MSDK/msdk/pull/571/files#r1184416416

Jake-Carter · 2023-05-02T23:32:02Z

Libraries/SDHC/ff13/Source/ffconf.h

@@ -171,6 +174,8 @@

 #define FF_MIN_SS		512
 #define FF_MAX_SS		512
+//#define FF_MIN_SS		4096


This needs a comment explaining in what situation the sector size of 4096 would be used.

We should also consider whether we want to wrap this in some variable that we have more control over from the build system.

Ex:

#ifdef SOMEVARIABLE #define FF_MIN_SS 512 #define FF_MAX_SS 512 #else #define FF_MIN_SS 4096 #define FF_MAX_SS 4096 #endif

Jake-Carter · 2023-05-02T23:38:47Z

Libraries/SDHC/ff13/Source/mscmem.c

+
+/**
+ * @file    mscmem.h
+ * @brief   Memory routines used by the USB Mass Storage Class example.


@sanjayjaroli can you take a look at the application of these files towards the SDHC library? Are there any maintenance considerations?

@MertCanBoz1 since FatFS is not USB-specific I wonder if using the Libraries/MiscDrivers/ExtMemory is a better choice?

It may be, but the external flash on the connectivity board of the MAXREFDES178 is different than the one used for EvKits. Some functions (specifically Ext_Flash_Erase.c ) need different handling than the ones at the mx25.c. The new AppSwitcher code calls mx25.c directly from the source file ( analogdevicesinc/MAX78xxx-RefDes#48 ).
Therefore, adding the mscmem.c to the Libraries/MiscDrivers/ExtMemory may cause some conflicts between them.

I think that's what I mean - we should use mx25.c directly. Ext_Flash.h provides a struct to abstract the physical communication layer which should be enough to address connectivity issues I think.

https://github.com/Analog-Devices-MSDK/msdk/blob/9bf68cf780920cc3d9654418c5722543f2e69c89/Libraries/MiscDrivers/ExtMemory/Ext_Flash.h#L143

Maybe I missed something - Does the cube camera present itself as a mass storage device for USB? Is that what the mscmem files are used for?

Jake-Carter · 2023-05-02T23:39:47Z

Libraries/SDHC/sdhc.mk

@@ -67,7 +67,7 @@ export PROJ_LDFLAGS
 export MXC_OPTIMIZE_CFLAGS
 export BOARD_DIR
 export USE_NATIVE_SDHC
-
+export FLASH


I suggest naming this variable EXTERNAL_FLASH

Jake-Carter

Looks good, thanks for implementing the changes.

I have one last minor suggestion below, but otherwise I think it's ready. Nice work!

Jake-Carter · 2023-05-10T15:53:58Z

Libraries/SDHC/ff13/Source/diskio.c

@@ -21,7 +24,7 @@
 #define INIT_CARD_RETRIES 10  

 /* Prototypes for module-only functions */
-static DRESULT ctrl_sync(void *buff);
+static DRESULT ctrl_sync(void *buff,BYTE pdrv);


I would suggest applying the same change that you made to ctrl_sync (adding pdrv as an argument) to get_sector_count, get_block_size, and mmc_get_csd as well. Then you can move the implementations inside of these functions with the same method that you have applied elsewhere.

Since these are internal module functions we can modify them more freely as we see fit.

This would help keep the implementation of the disk_ioctl function clean, so you wouldn't need the nested switch statements.

Jake-Carter · 2023-05-10T15:54:40Z

Libraries/SDHC/ff13/Source/diskio.c

-    }
-
+	    	DRESULT status;
+	switch(pdrv){


See comment above - pass this in as an argument to avoid nested switch statements

sihyung-maxim · 2023-05-17T14:58:50Z

/clang-format-run

sihyung-maxim · 2023-05-17T16:38:02Z

Not sure why the workflows aren't running...

This reverts commit db7e4ec.

sihyung-maxim · 2023-05-19T14:23:22Z

/clang-format-run

sihyung-maxim · 2023-05-19T15:01:41Z

The BTM-CI runner has not pulled the latest changes. Attempting to fix.

Libraries/MiscDrivers/ExtMemory/Ext_Flash.h

…_DUMMY

…v/sdhc

…ASH_READ_DUMMY" This reverts commit 675d646.

SDHC Library Changes to Enable New Features of the AppSwitcher Exampl…

c6fd1e9

…e (from refdes repository)

Jake-Carter requested changes May 2, 2023

View reviewed changes

MertCanBoz1 and others added 3 commits May 10, 2023 11:51

Merge branch 'Analog-Devices-MSDK:main' into dev/sdhc

bc42414

SDHC and ExtMemory Updates to Enable New Features of AppSwitcher

815863c

Delete ar_args.txt

0a884e5

Jake-Carter approved these changes May 10, 2023

View reviewed changes

MertCanBoz1 and others added 4 commits May 11, 2023 11:56

Minor Changes on diskio.c

4a74270

Merge branch 'Analog-Devices-MSDK:main' into dev/sdhc

dbe4336

ff13.c Minor Changes

ef2bdb5

Merge branch 'dev/sdhc' of github.com:MertCanBoz1/msdk into dev/sdhc

c9f0a46

Jake-Carter requested a review from sihyung-maxim May 16, 2023 15:23

clang-format bot reformatting.

db7e4ec

sihyung-maxim added 3 commits May 17, 2023 13:36

Updated copyright in sdhc.mk.

30a0c78

Merge branch 'main' into remote/MertCanBoz1/dev/sdhc

0acfdd2

Revert "clang-format bot reformatting."

3343faa

This reverts commit db7e4ec.

sihyung-maxim and others added 5 commits May 19, 2023 14:27

clang-format bot reformatting.

4c780c5

Triggering workflows.

121648c

Removed SDHC from linter check.

9bc31a8

Fixed linter errors.

0ecd3b2

Fixed linter errors.

aad15d9

AFE: Updated SVD file names to include Revision.

508bb9c

sihyung-maxim approved these changes May 22, 2023

View reviewed changes

Libraries/MiscDrivers/ExtMemory/Ext_Flash.h Show resolved Hide resolved

sihyung-maxim changed the title ~~SDHC Library Changes to Enable New Features of the AppSwitcher~~ feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC Jun 6, 2023

sihyung-maxim approved these changes Jun 14, 2023

View reviewed changes

sihyung-maxim added 2 commits June 14, 2023 13:27

style(Examples,MiscDrivers): Update capitalization for EXT_FLASH_READ…

675d646

…_DUMMY

Fix Linter filter

8dba49a

sihyung-maxim added 2 commits June 14, 2023 14:29

Merge remote-tracking branch 'origin/main' into remote/MertCanBoz1/de…

59224ef

…v/sdhc

Revert "style(Examples,MiscDrivers): Update capitalization for EXT_FL…

c99682c

…ASH_READ_DUMMY" This reverts commit 675d646.

sihyung-maxim merged commit ebf73d5 into analogdevicesinc:main Jun 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC #571

feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC #571

MertCanBoz1 commented Apr 28, 2023

Jake-Carter May 2, 2023

Jake-Carter May 2, 2023

Jake-Carter May 2, 2023 •

edited

Loading

MertCanBoz1 May 3, 2023

Jake-Carter May 4, 2023

Jake-Carter May 2, 2023

Jake-Carter May 2, 2023

MertCanBoz1 May 3, 2023

Jake-Carter May 2, 2023

Jake-Carter May 2, 2023

MertCanBoz1 May 3, 2023

Jake-Carter May 4, 2023

Jake-Carter May 2, 2023

Jake-Carter May 2, 2023

MertCanBoz1 May 3, 2023

Jake-Carter May 4, 2023

Jake-Carter May 2, 2023

Jake-Carter left a comment

Jake-Carter May 10, 2023

Jake-Carter May 10, 2023

sihyung-maxim commented May 17, 2023

sihyung-maxim commented May 17, 2023

sihyung-maxim commented May 19, 2023

sihyung-maxim commented May 19, 2023

feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC #571

feat(SDHC,MiscDrivers): Update AppSwitcher example to use new features with External Flash instead of SDHC #571

Conversation

MertCanBoz1 commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jake-Carter May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jake-Carter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihyung-maxim commented May 17, 2023

sihyung-maxim commented May 17, 2023

sihyung-maxim commented May 19, 2023

sihyung-maxim commented May 19, 2023

Jake-Carter May 2, 2023 •

edited

Loading