Config PIDs #4

PharosMarcusB · 2023-12-18T09:00:00Z

Replaces, enabled with the define ENABLE_CONFIG_PIDS, the DMX configuration mode with RDM PIDs.

The pixel type is set using DMX personality, as this can take advantage of the personality enumeration process.
The exisiting manufacturer pids have been enabled, tweaked, and extended with SETs (excluding 'Pixel Type' 0x8500).

'Pixel Map' (0x8503) is a small compromise due to RDMs lack of suitable custom parameter enumeration, as such entry is via ASCII text. e.g. 'RGB', 'rgb', 'BRG', et cetera; it remains quite useable with any RDM controller that allows string input.

Cherry pick items from 77cc817

This commit removes two two old source folders referenced by lib-gd32

lib-rdm was missing project include to lib-dmxreceiver. lib-configstore was missing project includes to lib-ws28xx, lib-ws28xxdmx, lib-rdm, and lib-rdmsensor.

Add references to main project to all sub-projects to aid Eclipse

Replace GCC function `__builtin_offsetof` with <cstddef> defined `offsetof`

Exposed by define `ENABLE_CONFIG_PIDS` replace the config mode personality with a separate personality for each supported pixel type.

Enable manufacturer pids and add the ability to set pixel type settings. Correct pixel parameter type sizes.

vanvught · 2023-12-18T09:20:56Z

lib-ws28xx/include/pixelconfiguration.h

These kind of parameters must be uint32_t. Otherwise additional ARM instructions will be generated reducing the size.

This somewhat depends on the extensions available on the specfic chip, I'm not really sure what the GD32 has.
However any small performance gain, in my mind, is outweighed by the convenience offered by C++ of correctly sized parameters.

correctly sized parameters.

Only when using 8/16-bit MCU's then it is best practices using 'correctly sized parameters.'. My open source software is targeted for 32-bit MCU's only. Therefore reducing parameters is not an option.

Would you have any aversions to using uint_fast8_t and uint_fast16_t, the true intended size is then obvious to the developer, but the complier then uses the best/fastest storage size for the processor.

Just a reference : https://community.st.com/t5/stm32-mcus-products/most-efficient-data-length/td-p/388385

I am not sure if using uint_fast8_t and uint_fast16_t would help in performance. Moreover, there is no performance issue with the firmware.

uint_fast8_t and uint_fast16_t resolve the to native platform register size; which on ARM mode would be 4 bytes (32 bit), but if the target CPU is operating in Thumb mode could be 2 bytes (16bit).
By using uint_fast8_t we are indicating to the compiler "use the native size", but to the developer "we don't intend this to exceed 255".

vanvught · 2023-12-18T09:31:12Z

lib-ws28xxdmx/include/ws28xxdmx.h

SetCount is handled in class PixelConfiguration. m_nGroups is set based on PixelConfiguration.GetCount()

GD32F303RC-DMX512-RDM/lib-ws28xxdmx/src/pixeldmxconfiguration.cpp

Line 75 in 39d9509

m_nGroups = GetCount() / m_nGroupingCount;

I'm not sure what you're referencing here, WS28xxDmx::SetCount() calls PixelConfiguration::SetCount()

The SetCount is validated at start-up. Within this validation several variables are calculated based on m_nCount.

PixelDmxConfiguration pixelDmxConfiguration; StorePixelDmx storePixelDmx; PixelDmxParams pixelDmxParams(&storePixelDmx); if (pixelDmxParams.Load()) { pixelDmxParams.Dump(); pixelDmxParams.Set(&pixelDmxConfiguration); } /* * DMX Footprint = (Channels per Pixel * Groups) <= 512 (1 Universe) * Groups = Led count / Grouping count * * Channels per Pixel * (Led count / Grouping count) <= 512 * Channels per Pixel * Led count <= 512 * Grouping count * * Led count <= (512 * Grouping count) / Channels per Pixel */ uint32_t nLedsPerPixel; pixeldmxconfiguration::PortInfo portInfo; pixelDmxConfiguration.Validate(1 , nLedsPerPixel, portInfo); if (pixelDmxConfiguration.GetUniverses() > 1) { const auto nCount = (512U * pixelDmxConfiguration.GetGroupingCount()) / nLedsPerPixel; pixelDmxConfiguration.SetCount(nCount); } WS28xxDmx pixelDmx(pixelDmxConfiguration);

Yes, unfortunately the way the application is currently structured this does mean a power cycle is necessary is needed to fully apply all the pixel properties.

unfortunately the way the application is currently structured this does mean a power cycle is necessary is needed to fully apply all the pixel properties.

Correct. That is the current limitation. I had the same discussion with Bas. It has a major effort changing the firmware in order to make it possible changing paramaters at run-time. I have done some pre-work in order to make it possible. So when we change for example the Count, then we need to run Validation(). And all run-time paramaters should be updated accordingly.

I am eager to get this fixed.

vanvught · 2023-12-18T09:46:33Z

lib-configstore/src/pixel/storepixeldmx.cpp

What would be the advantages for this change?

When we are going to change the global access for a Class then it must changed for all classes. And the impact, the usage of .bss, must be carefully analysed.
Currently there is just one 'singleton' implementation -> https://github.com/vanvught/GD32F303RC-DMX512-RDM/blob/development/lib-lightset/include/lightsetdata.h Which is introduced for making it possible defining the RAM section to be used.

The advantage is that it prevents the discipline needed to ensure only a single constructor was ever called.

The advantage is that it prevents the discipline needed to ensure only a single constructor was ever called.

Understood. I will analyse the impact and when possible I will change all classes in /lib-configstore across all solutions https://www.gd32-dmx.org/github.html
Thanks.

With regards the way the libs are stored, have you ever considered using git submodules?
e.g. have lib-configstore as a seperate git project, which is then imported as a submodule for each project.

It means that there is only a single code base for the lib, and each project then only needs to reference that. It would avoid the need to copy and merge source everytime there is a lib change.

have you ever considered using git submodules?

Yes. But I am missing the free time in order to make it happen. The base code is https://github.com/vanvught/rpidmx512 and I have./github-distribute.sh running in the background.

vanvught · 2023-12-18T10:15:48Z

Hi Marcus. thanks for these updates. I will work with it. We need to think about the implementation for changing the run-time properties. I am eager to get this done. I hope to find the time in the upcoming holidays. Thanks, Arjan

vanvught · 2023-12-20T11:21:44Z

Hi Marcus, I will introduce the configuration property DO_NOT_HAVE_CONFIGSTORE.
Then we can simplify the

GD32F303RC-DMX512-RDM/lib-ws28xxdmx/src/params/pixeldmxparams.cpp

Line 76 in 2dc17df

bool PixelDmxParams::Load() {

void PixelDmxParams::Load() {
	m_pixelDmxParams.nSetList = 0;

#if !defined(DISABLE_FS)
	ReadConfigFile configfile(PixelDmxParams::staticCallbackFunction, this);

	if (configfile.Read(DevicesParamsConst::FILE_NAME)) {
	   StorePixelDmx::Update(&m_pixelDmxParams);
	} else
#endif
	  StorePixelDmx::Copy(&m_pixelDmxParams);
#endif
}

And PixelDmxParamsRdm::PixelDmxParamsRdm(PixelDmxStore *pWS28xxDmxStore) can just be PixelDmxParamsRdm::PixelDmxParamsRdm(). Removing s_pWS28xxDmxStore from the Class.

This can be done for all *Params classes. Some time consuming edit, but the result is cleaner code.

The DO_NOT_HAVE_CONFIGSTORE is for some legacy software. Software which I should not support anymore.

vanvught · 2023-12-20T12:19:57Z

The DO_NOT_HAVE_CONFIGSTORE is for some legacy software. Software which I should not support anymore.

Given it a second thought. I will handle it in the legacy software. And not using DO_NOT_HAVE_CONFIGSTORE.

PharosMarcusB added 8 commits December 11, 2023 15:32

Fix project language settings

c9a920f

Cherry pick items from 77cc817

Remove source path references

4da3a7b

This commit removes two two old source folders referenced by lib-gd32

Add missing project include

f5b64aa

lib-rdm was missing project include to lib-dmxreceiver. lib-configstore was missing project includes to lib-ws28xx, lib-ws28xxdmx, lib-rdm, and lib-rdmsensor.

Add project references

f37e35e

Add references to main project to all sub-projects to aid Eclipse

Globally set define DISABLE_FS

58a9082

Replace __builtin_offsetof with offsetof

9b105e7

Replace GCC function `__builtin_offsetof` with <cstddef> defined `offsetof`

Add pixel type personalities

6eefa0a

Exposed by define `ENABLE_CONFIG_PIDS` replace the config mode personality with a separate personality for each supported pixel type.

Get/Set pixel type by RDM

885b51b

Enable manufacturer pids and add the ability to set pixel type settings. Correct pixel parameter type sizes.

vanvught reviewed Dec 18, 2023

View reviewed changes

vanvught merged commit 20bb08d into vanvught:development Dec 21, 2023
1 check passed

vanvught mentioned this pull request Jan 16, 2024

Make it possible to change run-time configuration properties #5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config PIDs #4

Config PIDs #4

PharosMarcusB commented Dec 18, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023 •

edited

Loading

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 19, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023

vanvught Dec 18, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023

vanvught Dec 18, 2023

PharosMarcusB Dec 18, 2023

vanvught Dec 18, 2023

vanvught commented Dec 18, 2023

vanvught commented Dec 20, 2023 •

edited

Loading

vanvught commented Dec 20, 2023

Config PIDs #4

Config PIDs #4

Conversation

PharosMarcusB commented Dec 18, 2023

Choose a reason for hiding this comment

PharosMarcusB Dec 18, 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

vanvught commented Dec 18, 2023

vanvught commented Dec 20, 2023 • edited Loading

vanvught commented Dec 20, 2023

PharosMarcusB Dec 18, 2023 •

edited

Loading

vanvught commented Dec 20, 2023 •

edited

Loading