drivers: Add zephyr,flash-disk compatible binding

Use devicetree node names instead of removed label property. This solves the storage init error when USB Mass Storage sample is configured to use FLASH-based disk.

Second patch is the wrong approach. Kconfig option should be removed entirely and DEVICE_DT_GET used instead. See devicetree release notes for v3.2 for extended discussion

No, that is not the intention of this patch. See also changes in tests/subsys/fs/fat_fs_api.

Second patch is the wrong approach. Kconfig option should be removed entirely and DEVICE_DT_GET used instead. See devicetree release notes for v3.2 for extended discussion
I can achieve this by modifying https://github.com/zephyrproject-rtos/zephyr/blob/11e33449093bc219f3ab31f8d036da05ae29c27a/drivers/disk/flashdisk.c#L59 to

#if DT_NODE_HAS_STATUS(DT_NODELABEL(mx25r64), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(mx25r64));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(gd25q16), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(gd25q16));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(flashcontroller0), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(flashcontroller0));
#endif

but doing so doesn't really seem right.
@mbolivar-nordic Is above what you want? How to handle the device selection, is the ifdef chain appropriate?

#if DT_NODE_HAS_STATUS(DT_NODELABEL(mx25r64), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(mx25r64));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(gd25q16), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(gd25q16));
#elif DT_NODE_HAS_STATUS(DT_NODELABEL(flashcontroller0), okay)
	flash_dev = DEVICE_DT_GET(DT_NODELABEL(flashcontroller0));
#endif

but doing so doesn't really seem right.

Is there an option to use chosen zephyr,flash-disk for that purpose? For example
chosen {
    zephyr,flash-disk = &mx25r64;
};

This would allow code like this
    flash_dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_flash_disk));

This is flexible and easily allows to set default value in DTS or board overlay, while at the same time easily allowing to replace it in app overlay by doing /delete-property/ zephyr,flash-disk on the chosen node.
There is also an option, less nice and flexible, to use specific label, for example
zephyr_flash_disk_0: &mx25r64 : { };

and in code:
   flash_dev = DEVICE_DT_GET(DT_NODELABEL(zephyr_flash_disk_0);

In both cases Kconfigs are no longer there and there is no need for if sequence, as flash disk selection is done in DTS.
IMHO using Kconfig to select flash disk is not best approach, because the Kconfig will require updates every time when new flash devices or boards with such devices are added; when the DTS is designated for selecting flash disk, and you add new board with new flash, all you have to do is to point to it with chosen or label, and you do not have to touch Kconfig, or the flash disk driver code.

Is there an option to use chosen zephyr,flash-disk for that purpose? For example
chosen {
    zephyr,flash-disk = &mx25r64;
};

This seems better than the #ifdef ladder but how to set the defaults? Should the zephyr,flash-disk (or equivalent) be specified in mass storage boards overlay? Should there be a native_posix.overlay specifying zephyr,flash-disk added to tests/subsys/fs/fat_fs_api?
Another option instead of overlays would be putting it by default in board dts, but it raises an issue what to do with targets that actually use the mx25r64 flash for some partitions? Going with the overlays in samples/test direction avoids this issue.

Should the zephyr,flash-disk (or equivalent) be specified in mass storage boards overlay? Should there be a native_posix.overlay specifying zephyr,flash-disk added to tests/subsys/fs/fat_fs_api?

Every sample and test, either generic or board specific, that uses mass storage needs to have overlay which elects the flash disk; the same way it does this with Kconfig now, just the place will be different.

Another option instead of overlays would be putting it by default in board dts, but it raises an issue what to do with targets that actually use the mx25r64 flash for some partitions? Going with the overlays in samples/test direction avoids this issue.

Not really fan of this, but it wouldn't break anything unless you enable all subsystems that use that specific flash at the same time.

I agree with the approach of zephyr,flash-disk and removal of Kconfig DISK_FLASH_DEV_NAME and replacing device_get_binding with DEVICE_DT_GET.

I have replaced DISK_FLASH_DEV_NAME Kconfig with zephyr,flash-disk chosen property, but it only seems a intermediate change. The next step would be to replace other Kconfig options (like DISK_VOLUME_SIZE) with zephyr,flash-disk device tree properties. Should the next step be handled as part of this PR or a next one?
A separate issue is handling multiple LUNs (that is multiple drives). Should the zephyr,flash-disk be changed to an array to allow specifying more than one disk?

I have replaced DISK_FLASH_DEV_NAME Kconfig with zephyr,flash-disk chosen property, but it only seems a intermediate change. The next step would be to replace other Kconfig options (like DISK_VOLUME_SIZE) with zephyr,flash-disk device tree properties. Should the next step be handled as part of this PR or a next one?

I think it is up to you. You will have to provide yml definition of flash-disk type device. @galak Is better source of information on that topic.

A separate issue is handling multiple LUNs (that is multiple drives). Should the zephyr,flash-disk be changed (once the flashdisk API is able to support more than one disk) to an array to allow specifying more than one disk?

This can be solved by adding "flash-disk" compatible and iterate over instance as it is done in case of other drivers.
Question is how do you want to identify the devices, or control order of them in the system, because even though this would be able to identify and configure device, how do we select one to do something with it?

@desowin, @de-nordic, would it not be better to point to a partition instead of a device and to use the macro's like in flash_map to get the device and the size information ? This would be more inline with the chosen,flash definitions.

@Laczen  Yes. That is true. Pointing directly to partition allows to pick all the information from flash partition configuration in DTS.

@desowin, @de-nordic, would it not be better to point to a partition instead of a device and to use the macro's like in flash_map to get the device and the size information ? This would be more inline with the chosen,flash definitions.

@Laczen Yes. That is true. Pointing directly to partition allows to pick all the information from flash partition configuration in DTS. There is still problem with multiple flash-disks, so maybe adding compatible parameter to flash partition, which would be something like:
compatible = 'zephyr,flash-disk'

would allow to identify all instances of flash disks this subsystem can serve.

@de-nordic, I don't know if the compatible= '...' is already used inside the partition declaration (e.g. to allow for linker usage), but I would rather go for a section in the dts that declares any flash-disks (with the `compatible = '...' declaration) and pointing to a partition. This IMHO is cleaner, but @galak might have a better/different view on this.

@de-nordic, I don't know if the compatible= '...' is already used inside the partition declaration (e.g. to allow for linker usage), but I would rather go for a section in the dts that declares any flash-disks (with the `compatible = '...' declaration) and pointing to a partition. This IMHO is cleaner, but @galak might have a better/different view on this.

Yes, this is also valid solution.

Can we follow @Laczen suggestion and point to partition instead of device?
This will allow us also to pick following characteristics, using flash map macros:
CONFIG_DISK_FLASH_START, CONFIG_DISK_VOLUME_SIZE. It should be also possible to get up the tree from the partition and find CONFIG_DISK_ERASE_BLOCK_SIZE, CONFIG_DISK_FLASH_ERASE_ALIGNMENT (which seem to be the same thing?).
The only Kconfig I have no substitute for is CONFIG_DISK_FLASH_MAX_RW_SIZE.

flash_disk: disk0 {
  compatible = "zephyr,flash_disk";
  partition = &storage;
  max-rw-size = 512;
  other things if needed
};

Gotcha.  Wonder if we can come up with something a bit more generic that could cover RAM and SD cases as well:
disk0 {
   compatible = "zephyr,disk";
   storage-dev = &storage;
...
};

Maybe something where there is a base zephyr,disk and than also zephyr,flash-disk, zephyr,ram-disk, etc..

Gotcha. Wonder if we can come up with something a bit more generic that could cover RAM and SD cases as well:
disk0 {
   compatible = "zephyr,disk";
   storage-dev = &storage;
...
};

Maybe something where there is a base zephyr,disk and than also zephyr,flash-disk, zephyr,ram-disk, etc..

YES, this should be possible, the storage-dev should allow to be: a device, a partition, a ram buffer (assigned in memory section) depending on zephyr,sd-disk, zephyr,flash-disk, zephyr,ram-disk compatible.

Can we follow @Laczen suggestion and point to partition instead of device? This will allow us also to pick following characteristics, using flash map macros: CONFIG_DISK_FLASH_START, CONFIG_DISK_VOLUME_SIZE. It should be also possible to get up the tree from the partition and find CONFIG_DISK_ERASE_BLOCK_SIZE, CONFIG_DISK_FLASH_ERASE_ALIGNMENT (which seem to be the same thing?). The only Kconfig I have no substitute for is CONFIG_DISK_FLASH_MAX_RW_SIZE.

That was tried in #33657 long time ago. I reopened it, and have hacked disk bindings proposal in there, not sure if it convinces me.

then I have to try to learn whatever specifics of the subsystem are that I'm not familiar with

... follow-up comment here. I went and read

the manually written API documentation in https://docs.zephyrproject.org/latest/services/storage/disk/access.html#disk-access-api
the API docstring comments in https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/disk.h
the sample documentation in https://docs.zephyrproject.org/latest/samples/subsys/usb/mass/README.html

And I'm still not really clear on what the problem is we are trying to solve (besides a build error). It would really help if this could be improved. "RTFS" is not a great way for me to get up to speed quickly on what's going on.

@mbolivar, the problem that is being solved is the configuration of disks (sd, flash or ram based). This was never properly done, it instead uses some config options that would use the labels to generate information.
To do it properly the solution should be:

multi-instance from the start,
allow concurrent use of all types at the same time,
not use labels,
support all device types (sd, flash, ram) at the same time,

This could be done in several ways, by adding compatibles to partitions, devices, rambuffer or by creating a node that contains a link to the specific back-end. The latter seems to me the best solution as it allows adding extra information that otherwise would need to be added inside e.g. a partition node.

@Laczen thanks for that comment but it's still not really clear to me what the problem is. Let me try to ask some follow-up questions.
First: the disk API is a bit "weird" and unlike other device APIs, because it uses a struct disk_info to wrap a struct device, so what are the "instances" here? Instances of device? Of struct disk_info? The API documentation is not clear on how it fits together. Can 2 disk_info structs share the same device struct? If so, which one is the "instance" you are speaking of? None of the documentation links I pasted above seem to clarify that. Is this just undocumented, or did I miss something?
Second, in Zephyr we usually use a chosen node to describe a system-wide setting, and it's not clear to me how that plays into your "multi-instance" comment. I don't see anything in include/zephyr/drivers/disk.h that looks like it would require a system-wide setting. In https://docs.zephyrproject.org/latest/build/dts/api/api.html#zephyr-specific-chosen-nodes you can see how chosen nodes are used to configure which device out of many options should be used by a subsystem in several cases, such as:
zephyr,bt-uart | Sets UART device used by Bluetooth
Here, the Bluetooth subsystem needs to know which UART to use (for HCI, I guess).
And the application can do something with whatever device corresponds to a chosen node as well, since everybody knows the whole devicetree.
Third, it's not clear to me why either a chosen node or a node that contains a link to the specific back-end is a good solution to a situation where you need to "allow concurrent use of all types at the same time" and "support all device types (sd, flash, ram) at the same time". Again, the one disk device (or disk_info?) vs. many problem is not being specified here in the PR.
Please remember that you are the experts in the disk API, so as an outsider on the devicetree side, it's hard to understand if a representation is good when I don't have that expert knowledge and especially if the docs are not clear.

@mbolivar-nordic, when disks would be defined as proposed there would be no need for the chosen node solution. I hope this answers questions two and three.
Regarding question 1, allow me not to comment on this, I am not the disk expert nor the maintainer.

@Laczen thanks for that comment but it's still not really clear to me what the problem is.

The USB mass storage example does not work in Zephyr 3.2.0 when built with -DCONFIG_APP_MSC_STORAGE_FLASH_FATFS=y. It worked fine in earlier releases, therefore it is a regression.
The naive fix is to change the "MX25R64" to "mx25r6435f@0" and "GD25Q16" to "gd25q16@0" in samples/subsys/usb/mass/Kconfig. It would solve the regression, i.e. the USB mass storage example would just work.
Your comment about the need to remove Kconfig option essentially discarded the "simply patch the regression" option and led to discussion what would be the correct approach.
The naive #ifdef chain is questionable as it would just get rid of the Kconfig option but would still leave the underlying issue intact. The zephyr,flash-disk solution seems actually harmful as it adds a chosen node that is already known to be flawed at the time of introduction.

First: the disk API is a bit "weird" and unlike other device APIs, because it uses a struct disk_info to wrap a struct device, so what are the "instances" here? Instances of device? Of struct disk_info? The API documentation is not clear on how it fits together. Can 2 disk_info structs share the same device struct? If so, which one is the "instance" you are speaking of? None of the documentation links I pasted above seem to clarify that. Is this just undocumented, or did I miss something?

From USB point of view, the "instance" refers to LUN (logical unit). The host accesses each LUN as block device. It is not necessary to expose whole flash device as a LUN. The pretty simple use case would be to have part of flash chip containing user data and the other part to hold some assets like firmware graphics, blobs or even actual code. Therefore the partition approach instead of disk approach is pretty appealing.

Lets net overdue this, currently we have nodes for flash, partition, Kconfig for disk and Kconfig for file system, if there would be any put on top of the disk.
As an example, SoC flash definition from dts/arm/nordic/nrf52840.dtsi:
371                         flash0: flash@0 {
372                                 compatible = "soc-nv-flash";
373                                 erase-block-size = <4096>;
374                                 write-block-size = <4>;
375                         };
376                 };

Partition on this flash0:
254 &flash0 {
...
283                 /*
284                  * Storage partition will be used by FCB/LittleFS/NVS
285                  * if enabled.
286                  */
287                 storage_partition: partition@f8000 {
288                         label = "storage";
289                         reg = <0x000f8000 0x00008000>;
290                 };
291         };

Then Kconfigs for disk:
CONFIG_DISK_FLASH_DEV_NAME
CONFIG_DISK_FLASH_START
CONFIG_DISK_ERASE_BLOCK_SIZE
CONFIG_DISK_FLASH_ERASE_ALIGNMENT
CONFIG_DISK_FLASH_MAX_RW_SIZE
CONFIG_DISK_VOLUME_SIZE

and finally FAT FS configuration:
CONFIG_FS_FATFS_MAX_SS

Above values affect how is the data writes/reads/erases handled by subsystems; DTS entries affect disk access without placing system over it, Kconfig affect file system operation.
What DTS misses is read alignment and minimal size, which is characteristic of, at least, some external devices (look at CONFIG_NORDIC_QSPI_NOR_STACK_WRITE_BUFFER_SIZE).
FAT FS uses CONFIG_FS_FATFS_MAX_SS to set sector for read/write window, and this is MAX value so if system will mount devices with sectors that are 512 and 4096, this should be 4096. The erase-block-size value is used only when mkfs operation is performed and is written to FAT FS characteristics on a device. This is done by calling disk_access_ioctl with DISK_IOCTL_GET_SECTOR_SIZE command.
When dealing with moving disk definition to DTS we need to avoid taking over what is "software configuration" and adding redundant configuration items.
For disk placed on fixed-partition, we can take CONFIG_DISK_FLASH_START and CONFIG_DISK_VOLUME_SIZE from partition,
the CONFIG_DISK_ERASE_BLOCK_SIZE should be taken from controller and CONFIG_DISK_FLASH_ERASE_ALIGNMENT is redundant, as it can not be different than CONFIG_DISK_ERASE_BLOCK_SIZE; CONFIG_DISK_FLASH_MAX_RW_SIZE does not make sense and should be replaced with write-alignment, although this is problematic as in case of NOR devices this thing is actually configured in Kconfig, not DTS.
Ths means that CONFIG_DISK_FLASH_START, CONFIG_DISK_VOLUME_SIZE, CONFIG_DISK_FLASH_ERASE_ALIGNMENT, CONFIG_DISK_ERASE_BLOCK_SIZE can be already dropped, because they can be extracted as characteristics superimposed on flash disk, by nodes it derived from.
Whatever will be using flash disk should be given information on the raw device characteristic underneath and then device, as minimal required: minimal erase size, minimal write alignment, etc. Then the software may have modifiers (multipliers) to these values exposed by Kconfig, if it is needed.
For exmaple FAT FS Kconfig CONFIG_FS_FATFS_MAX_SS could be multiply of what flash device is providing as erase-block-size.
Side note:  I also think that we may have bug here, because there is also MIN value for that (in ffconfg.h), and when they differ then FAT FS uses disk_ioctl  command  DISK_IOCTL_GET_SECTOR_SIZE  to get block size for a device at run-time, which allows to apply different (no bigger than MAX) block sizes for different devices, and we set both values to the same thing.
I think that this is maximum what disk entry should provide:
flash_disk: disk0 {
  compatible = "zephyr,flash_disk";
  partition = &storage;
};

@de-nordic What you described pretty much aligns what is done in #33657. If #33657 gets merged, this PR will be obsolete.

@de-nordic What you described pretty much aligns what is done in #33657. If #33657 gets merged, this PR will be obsolete.

Sorry, I do not remember that PR, even though I have been reviewing it.

Just catching up on the conversation, but if I'm understanding correctly the core issue here is that we need a generic way to describe a region of flash that the flash disk driver should use. If that's the case, I generally agree with what @de-nordic has outlined in this comment, but question if we need a flash-disk chosen node, or could use a flash partition directly.
With that we have a few potential restrictions, as @de-nordic mentioned:

minimum write size
minimum erase size

We also of course need to know where in the flash device this region resides, and the size (both can be encoded by a flash partition)
In the case of the flash driver, we have the flash_get_parameters API available, which would permit the flash disk driver to get the minimum write size. Since write size is queried via the disk_access_ioctl API, we could get this parameter at runtime.
With that, could we do something like the following?
chosen {
    zephyr,storage-partition = &storage_partition;
};
...
&flash0 {
   partitions {
       ....
       storage_partition: partition@1e000 {
		label = "storage";
		reg = <0x0001e000 0x00002000>;
	};
   };
};

If we always use a partition within a flash device, then we can reference the parent flash controller in the flashdisk.c driver, and call flash_get_parameters in order to get properties like the minimum flash write size. As a bonus, this would offer a benefit to other samples (such as the littlefs one) that use a storage_partition directly by its nodelabel

@danieldegrasse, wouldn't the last proposal limit the number of disks in flash to only one ? I think it would be better not to use a chosen node to allow definition of disks in flash/ram/... all at the same time.

It would, that's a good point. I see the argument for a defined flash disk compatible now. What about using this flash-disk compatible on storage partitions themselves, and adding properties like the disk name there as well? I am ok with adding a new devicetree node to describe flash disks if required, but my concern is that we will end up describing properties related to the flash (min write size, min erase size) that are already available if we use a storage partition directly, and I want to be sure we avoid duplication of that data

I agree that we should avoid duplicate definitions, but are they really duplicating? Can we always be sure that the flash definitions make sense for a disk definition? Let's look at some examples:
erase-block-size = 4096, write-block-size = 4 -> flash disk block-size = 512, flash disk erase-block-size = 4096
erase-block-size = 256, write-block-size = 32 -> flash disk block size = 512, flash disk erase-block-size = 512

We can try to add this logic to the flash disk driver, but probably it is just easier to specify them and just to check the validity.

In current PR version, the only thing that can be classified as duplication is CONFIG_DISK_ERASE_BLOCK_SIZE. It defaults to 0x1000 and is checked at runtime (in disk_flash_access_init()) whether it is large enough. Automatically picking up the value is a bit problematic as it has to be maximum of all zephyr,flash-disk and considering that qspi devices do not have directly available erase-block-size it is hard to derive it at compile time.

@desowin, would it not be better to define the readcopy buffer for each instance and the flash_disk_data to be extended with:

a pointer to the readcopy buffer
the mutex

@desowin, would it not be better to define a readcopy buffer for each instance and the flash_disk_data to be extended with:
* a pointer to the readcopy buffer
* the mutex

I don't really see much benefit with doing so. The buffer is only used during writes (and only when writing less than a full page) and USB Mass Storage class cannot multiplex write commands to multiple LUNs (i.e. previous write must be finished before new one can be issued). Therefore, the USB Mass Storage would not see any benefit of having multiple buffers.

@desowin, would it not be better to define the readcopy buffer for each instance and the flash_disk_data to be extended with:

a pointer to the readcopy buffer
the mutex

@desowin, would it not be better to define a readcopy buffer for each instance and the flash_disk_data to be extended with:

a pointer to the readcopy buffer
the mutex

I don't really see much benefit with doing so. The buffer is only used during writes (and only when writing less than a full page) and USB Mass Storage class cannot multiplex write commands to multiple LUNs (i.e. previous write must be finished before new one can be issued). Therefore, the USB Mass Storage would not see any benefit of having multiple buffers.

I agree that USB Mass Storage would not benefit from it, but other users might.
When there is only one flashdisk it does not matter if the readcopy buffer is a global variable or defined for a disk, and the only addition is one pointer.
When there are multiple drives you avoid the problem of having a globally defined readcopybuf size and using one globally defined mutex. If the mutex is only used to regulate access to the readcopybuf it would be OK, if it is not it is better to provide a mutex for each flashdisk. There is a clear advantage to using one readcopybuf (used memory), but I doubt that it is a good idea to insert this limitation by default.
If it is really unnecessary to have multiple readcopybufs it should be possible to walk over all dts definitions to find the maximum space required and then to define the size. By defining this property at the dts level it is possible to find the maximum size easily and a runtime check can still be added for devices that do not have it defined it the dts.

If the mutex is only used to regulate access to the readcopybuf it would be OK, if it is not it is better to provide a mutex for each flashdisk

Yes, the mutex is used only to regulate access to read_copy_buf.

If it is really unnecessary to have multiple readcopybufs it should be possible to walk over all dts definitions to find the maximum space required and then to define the size. By defining this property at the dts level it is possible to find the maximum size easily and a runtime check can still be added for devices that do not have it defined it the dts.

So you mean that the erase-block-size property should be added to zephyr,flash-disk compatible? How to walk the dts to find the maximum property value of compatible instances?

In current PR version, the only thing that can be classified as duplication is CONFIG_DISK_ERASE_BLOCK_SIZE. It defaults to 0x1000 and is checked at runtime (in disk_flash_access_init()) whether it is large enough. Automatically picking up the value is a bit problematic as it has to be maximum of all zephyr,flash-disk and considering that qspi devices do not have directly available erase-block-size it is hard to derive it at compile time.

This could be renamed CONFIG_DISK_MAX_SUPPORTED_ERASE_BLOCK_SIZE, with better description how it affects system.
We need this buffer anyway whether disk with run-time provided or compile-time provided erase block size is used.
At this point this is property of software, that allows it to work with devices with different characteristics.
Note also that erase block size may be stored within file system information on mkfs. What should then a device with DTS setting erase-block-size do when memory device with such file system is connected to system?
Setting erase-block-size with DTS is giving disk certain hardware characteristics, which is not true for devices like SPI connected memories where erase block size can be often configured at run time.
When you connect flash disk on SPI and the flash-disk node states that the disk has erase-block-size 4096B, but file system configuration read from disk states 16B, what does the system should do now?

If the mutex is only used to regulate access to the readcopybuf it would be OK, if it is not it is better to provide a mutex for each flashdisk

Yes, the mutex is used only to regulate access to read_copy_buf.

If it is really unnecessary to have multiple readcopybufs it should be possible to walk over all dts definitions to find the maximum space required and then to define the size. By defining this property at the dts level it is possible to find the maximum size easily and a runtime check can still be added for devices that do not have it defined it the dts.

So you mean that the erase-block-size property should be added to zephyr,flash-disk compatible? How to walk the dts to find the maximum property value of compatible instances?

This should be possible with some macro wizardry and the for-each macro. I haven't done this yet, but it should be possible. However before trying to do this the question should be answered if a shared buffer is allowed or not: I think there can be situations were 2 flashdisks need access to the readcopy buffer at the same time (e.g. when copying from/to each other) and this might require two buffers to avoid a lockup. This would mean that each disk really needs its own buffer.

@de-nordic, this is just a naming problem. If in the dts this is specified as: read-write-copy-buffer-size it is exactly what it is used for and it can be easily validated if the size is ok. How much of the buffer is used can then be determined in run-time.

This could be renamed CONFIG_DISK_MAX_SUPPORTED_ERASE_BLOCK_SIZE, with better description how it affects system. We need this buffer anyway whether disk with run-time provided or compile-time provided erase block size is used. At this point this is property of software, that allows it to work with devices with different characteristics.

It should be maximum value from the set of minimum erase block sizes of all zephyr,flash-disk partitions. The minimum erase block size is determined by hardware.

Note also that erase block size may be stored within file system information on mkfs.

The filesystem is completely transparent to zephyr,flash-disk. The flashdisk is essentially a block device wrapper over flash devices. The need for the read_copy_buf is due to underlying hardware limitation (can only erase in whole blocks, i.e. there is no way for software to erase less bytes at a time), not a software one (filesystem or whatever).
If flash_erase() cannot erase the page_size bytes (not less not more) as returned by flash_get_page_info_by_offs() then I would consider the problem to be in the flash driver.

What should then a device with DTS setting erase-block-size do when memory device with such file system is connected to system?

Not care at all. The flashdisk layer does not need such information.

Setting erase-block-size with DTS is giving disk certain hardware characteristics, which is not true for devices like SPI connected memories where erase block size can be often configured at run time.

The "configured at run time" is very misleading. The software can use one of supported erase sizes. The minimum possible is what is relevant to flashdisk driver (if the zephyr,flash-disk partition spans across non-homogeneous sized sectors, then it is the maximum value of the minimum possible erase size of all sectors used by the partition).

When you connect flash disk on SPI and the flash-disk node states that the disk has erase-block-size 4096B, but file system configuration read from disk states 16B, what does the system should do now?

Filesystem is different layer. Flashdisk does not and should not care what data it holds.

However before trying to do this the question should be answered if a shared buffer is allowed or not: I think there can be situations were 2 flashdisks need access to the readcopy buffer at the same time (e.g. when copying from/to each other) and this might require two buffers to avoid a lockup. This would mean that each disk really needs its own buffer.

The only situation when you would need two flashdisks to access read_copy_buf is when writing to both at the same time. The example you provide does not require simultaneous access to read_copy_buf. The read_copy_buf is not needed nor used when **reading.
The way read_copy_buf is used is:

acquire read_copy_buf_lock
read flash memory data into read_copy_buf. This is necessary because the write will only partially modify the page, but the hardware can only erase no less than page_size bytes at a time (e.g. writing only 1 sector out of 8). The code actually reads more than strictly necessary, but that keeps the code simple (optimization is definitely out of this PR scope).
Put the new data in read_copy_buf, so the read_copy_buf contains the desired state on disk
Write the data to flash
Release read_copy_buf_lock

As the data to be written is already held somewhere in RAM by the time read_copy_buf_lock is acquired, I see no possibility for a lockup.

As the data to be written is already held somewhere in RAM by the time read_copy_buf_lock is acquired, I see no possibility for a lockup.

Of course I meant when both would be writing at the same time. Maybe there is no possibility of a lock-up but I am not sure. It feels like a needless complication to have only one buffer. When there is only one flashdisk it doesn't matter, if there are more it is more straightforward to define a buffer for each. It is clean and avoids complications. The mutex would become a write lock for the flashdisk.

As the data to be written is already held somewhere in RAM by the time read_copy_buf_lock is acquired, I see no possibility for a lockup.

Of course I meant when both would be writing at the same time. Maybe there is no possibility of a lock-up but I am not sure. It feels like a needless complication to have only one buffer. When there is only one flashdisk it doesn't matter, if there are more it is more straightforward to define a buffer for each. It is clean and avoids complications. The mutex would become a write lock for the flashdisk.

I don't really think the code complexity changes at all if there is one or multiple buffers involved. In my opinion the reduced RAM usage justifies having just one buffer.
The only reason for having per instance buffer would be writeback caching (to limit number of erases when subsequent sectors are written) but that is optimization and definitely way out of the scope of this PR (which is to solve the hardware description problem). But even with caching, it would still probably work just fine with just one buffer.

As the data to be written is already held somewhere in RAM by the time read_copy_buf_lock is acquired, I see no possibility for a lockup.

Of course I meant when both would be writing at the same time. Maybe there is no possibility of a lock-up but I am not sure. It feels like a needless complication to have only one buffer. When there is only one flashdisk it doesn't matter, if there are more it is more straightforward to define a buffer for each. It is clean and avoids complications. The mutex would become a write lock for the flashdisk.

I don't really think the code complexity changes at all if there is one or multiple buffers involved. In my opinion the reduced RAM usage justifies having just one buffer.
The only reason for having per instance buffer would be writeback caching (to limit number of erases when subsequent sectors are written) but that is optimization and definitely way out of the scope of this PR (which is to solve the hardware description problem). But even with caching, it would still probably work just fine with just one buffer.

I just don't like these hidden dependencies between flashdisks, if it is really worthwhile to optimize this, define a buffer in the dts and use this buffer for each of the flashdisks. Make it clear that this is happening and not somewhere hidden in code. But that is just my view.

As the data to be written is already held somewhere in RAM by the time read_copy_buf_lock is acquired, I see no possibility for a lockup.

Of course I meant when both would be writing at the same time. Maybe there is no possibility of a lock-up but I am not sure. It feels like a needless complication to have only one buffer. When there is only one flashdisk it doesn't matter, if there are more it is more straightforward to define a buffer for each. It is clean and avoids complications. The mutex would become a write lock for the flashdisk.

I don't really think the code complexity changes at all if there is one or multiple buffers involved. In my opinion the reduced RAM usage justifies having just one buffer.
The only reason for having per instance buffer would be writeback caching (to limit number of erases when subsequent sectors are written) but that is optimization and definitely way out of the scope of this PR (which is to solve the hardware description problem). But even with caching, it would still probably work just fine with just one buffer.

I just don't like these hidden dependencies between flashdisks, if it is really worthwhile to optimize this, define a buffer in the dts and use this buffer for each of the flashdisks. Make it clear that this is happening and not somewhere hidden in code. But that is just my view.

BTW are you sure that it is allowed for two threads to write at the same time when there is no access to the buffer ?

I just don't like these hidden dependencies between flashdisks, if it is really worthwhile to optimize this, define a buffer in the dts and use this buffer for each of the flashdisks. Make it clear that this is happening and not somewhere hidden in code. But that is just my view.

Defining it in devicetree seems like a good idea, but how to actually define it? I guess there would be some buffer property in zephyr,flash-disk compatible, but to what it would point to?

I just don't like these hidden dependencies between flashdisks, if it is really worthwhile to optimize this, define a buffer in the dts and use this buffer for each of the flashdisks. Make it clear that this is happening and not somewhere hidden in code. But that is just my view.

Defining it in devicetree seems like a good idea, but how to actually define it? I guess there would be some buffer property in zephyr,flash-disk compatible, but to what it would point to? Could you please show example what it would look like?

See tests/lib/devicetree/memory_region, the compatible would just have a phandle to the memory region. And you can find out all sizes.

I just don't like these hidden dependencies between flashdisks, if it is really worthwhile to optimize this, define a buffer in the dts and use this buffer for each of the flashdisks. Make it clear that this is happening and not somewhere hidden in code. But that is just my view.

Defining it in devicetree seems like a good idea, but how to actually define it? I guess there would be some buffer property in zephyr,flash-disk compatible, but to what it would point to? Could you please show example what it would look like?

See tests/lib/devicetree/memory_region, the compatible would just have a phandle to the memory region. And you can find out all sizes.

I don't think memory region is supposed to be used that way.
I think the DISK_ERASE_BLOCK_SIZE Kconfig option is really most appropriate here. It could be removed if the devicetree zephyr,flash-disk node would get the erase block size property, but then there is the potential duplication.
@mbolivar-nordic what do you think of it? How to handle the RAM buffer need (ideally at compile time, without resorting to runtime dynamic memory allocation)?

@galak @danieldegrasse can you please take another look?

@desowin, doesn't the flashdisk need a mutex to protect against simultaneous writes from different threads ?

@desowin, doesn't the flashdisk need a mutex to protect against simultaneous writes from different threads ?

It is out of scope of this PR - flashdisk does not have multi thread access control and this PR does not address it. This PR is merely about specifying the storage partition. The actual reason behind this PR is that current Kconfig configuration is incorrect.

@desowin, there are two things (apart from the eraseblock at config level) I don't like about the PR:
a. The sector_size is defined at config level, forcing all instances to use the same value,
b. The if (rc < 0), this generates larger code than if (rc != 0).
But I am not going to block the PR with a request for changes.

The sector_size is defined at config level, forcing all instances to use the same value

I think this can be solved by introducing max-page-size property that should be set to the size of largest page in flash area (currently all pages are assumed to be of equal size). It makes sense because the caching is really necessary and implementing multidisk caching with single buffer is problematic.

The sector_size is defined at config level, forcing all instances to use the same value

I think this can be solved by introducing max-page-size property that should be set to the size of largest page in flash area (currently all pages are assumed to be of equal size). It makes sense because the caching is really necessary and implementing multidisk caching with single buffer is problematic.

Isn't the sector size the size that is reported as the block size for a classical block device? It is a value that can be chosen and there is probably no need to align it to the flash properties.

Isn't the sector size the size that is reported as the block size for a classical block device? It is a value that can be chosen and there is probably no need to align it to the flash properties.

Ah, I misread what you wrote. I thought it was about the (erasable) page size that is determined by hardware, and hence the answer.
The sector size can be chosen, but not freely. For correct operation (assuming we don't want to lose capacity due to padding), some integer multiple of sector size must be equal to page size.

Apparently getting maximum page size (erase block size) for all compatibles is easier than I thought. All that has to be done is to declare an union with buffer arrays (each union entry is array holding one disk page size) and get a sizeof of such union.
I think it makes sense to get page size (required) and sector size (optional, default 512) properties to device-tree. as part of this PR. It would get rid of two more Kconfig options that really should be per-flashdisk instance and not shared.

@desowin, regarding the name of the page-size property, I propose to call it the read-modify-write-buffer-size or even just buffer-size do not call it anything related to pages or erase-blocks this would just make it confusing.

A small nitpick: maybe you can directly define the buffer as a union of the different buffers instead of first calculating the size. This might remove warnings on non-used _buf##n.

Where do you see warnings about non-used _buf##n? It is just an anonymous union. I added -Werror to CONFIG_COMPILER_OPT and it compiled just fine.

A small nitpick: maybe you can directly define the buffer as a union of the different buffers instead of first calculating the size. This might remove warnings on non-used _buf##n.

Where do you see warnings about non-used _buf##n? It is just an anonymous union. I added -Werror to CONFIG_COMPILER_OPT and it compiled just fine.

If they would be declared static I think they would trigger this.

@galak @mbolivar-nordic May we please get +1 from devietree perspective?

@galak @mbolivar-nordic Do you have any more comments or requests?

@desowin, Thank you for the excellent work and the perseverance.


Only use -isystem for LLVM if the directory is not already on the path

Building on Arch Linux, using gcc 6.1.1 and LLVM 3.7.1, fails with
the error:
/usr/include/c++/6.1.1/cmath:45:23: fatal error: math.h: No such file or directory
#include_next <math.h>
Avoiding the use of -isystem if the LLVM include directory is already
on the search path should avoid the problem.
This fixes #797

The one CI error looks unrelated, but I'm running that matrix item again to be sure.

Looks good. Nice work @tmmcguire.
@jemc you good with merging?


解决了API模式下单个文件扫描无法获取文件内容的问题

Pull Request Test Coverage Report for Build 404

6 of 9 (66.67%) changed or added relevant lines in 2 files are covered.
9 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.07%) to 42.36%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

cobra/engine.py
5
8
62.5%

Files with Coverage Reduction
New Missed Lines
%

cobra/utils.py
1
35.51%

cobra/log.py
1
44.2%

cobra/init.py
1
30.0%

cobra/api.py
2
12.92%

cobra/config.py
2
61.04%

cobra/cve.py
2
80.94%

Totals

Change from base Build 403:
0.07%

Covered Lines:
1558

Relevant Lines:
3678

💛  - Coveralls


Cleanup rtmemref api

-  use forward declaration to hide private data fields
-  RtMemRef.h: external user header, C/C++
- _RtMemRef.h: internal user header, C++ only
-  RtMemRef.hpp and RtMemRef.cpp: implementation header and file
-  add external APIs
       OrderedRtMemRefDict *ormrd_create(RtMemRef **rmrs, int n)
       RtMemRef **ormrd_getRmrs(OrderedRtMemRefDict *ormrd)
       int ormrd_getNumOfRmrs(OrderedRtMemRefDict *ormrd)
   for creating and querying OrderedRtMemRefDict with RtMemRef arrays
-  data buffer installed by rmr_setData() will be managed by user
-  unique_ptr<RtMemRef> must use custom deleter <RtMemRef,decltype(&rmr_destroy)>

@AlexandreEichenberger thanks! working on it.

@gongsu832 my updates:

Rename OrderedRtMemRefDict -> RtMemRefList for simplicity. One can still sort of use RtMemRefList like a dictionary (keyed by rmr names) since we can iterate over the list of rmrs and find the first match instead of relying on an actual dictionary.
Change API names to conform to the naming styles used by the rest of the project (LLVM style).
Clarify memory management responsibilities and create constructors that allow user to specify data ownership.
Apply alex's comments to build a database of data type metadatas.

I'm still working on the docs, but these are all my code changes; I hope to hear your opinions.

@gongsu832, my response to the high-level question of why I dropped the distinction between external/internal APIs: I think some internal api's are kept internal because communicating its semantics to users is hard and one can get oneself into an inconsistent state if it is used. However, the same rationale applies to stop onnx-mlir developers to use these troublemakers. So I removed some of them (like OMTensorList mutator) and made rest of them public.
Do you see other compelling reasons to have a internal/external separation? The difference between C/C++ corresponds loosely with the current designation of external/internal APIs and that's because we haven't introduced C++ APIs yet.

@tjingrant I think we do have to make a distinction between end users and developers. Developers can be educated to use the internal APIs properly. In a sense, they actually have to. End users on the other hand should not be expected to know anything that are not meant for them.
Also purely from a good coding practice point of view, if something is not meant for the end user, then it should not appear in any user visible API header. So as much as I hate to split the header, I don't see a better way around this unless we advertise those internal APIs as end user callable APIs, which in my opinion is really not what we want to do.

@gongsu832 how should we decide which API is for users to know and which not? I tried to tell the difference, but for every API function in this patch, I can always think of (maybe rare but still valid) reasons that users may still want to use it: just to give you some examples,

omTensorCreateWithRandomData is useful if users decide to benchmark their model/device with random input.
omTensorAreTwoOmtsClose is useful if users decide to check whether the model is compiled correctly by comparing it with the reference value they obtained through PyTorch or Tensorflow.

Furthermore, I think we do not have to choose between:

Keeping things simple and not to overwhelm users.
Making available as many bells and whistles to user as we have, to save user time/effort in case they need it.

Because the former is documented through tutorials/how-tos, and the latter is documented through references, and I do keep them separate (tutorial is included as in OnnxMlirRuntime.h, which is translated to HTML pages through doxygen).
Taking a step back, I actually don't know what is the motivation for having an internal API at this point - now that those trouble-maker API functions are either removed or deprecated. If there is a good motivation, it must entail a simple rule that divides these APIs into two classes (user-facing and developer-facing). If this rule of separation is complicated, then it's actually worse off for users and developers alike, because then users/developers will have to find them from two, instead of one places.
Taking another step back, if what you mean by developer-facing API functions are helper functions like computeStridesFromSizes then I totally agree they should be moved out of the user-facing headers; in fact I think they shouldn't be in any header files at all and I was being sloppy when I put them in OMTensor.h.

@tjingrant I think the rule is fairly simple. However we decide which functions are useful for end users and should be exposed, they stay in the user visible headers; otherwise, they don't. I think that's a fairly standard coding practice.
So for the examples you gave, they stay in user visible headers. But what about calls like

omTensorGetAlignedPtr
omTensorSetPtr

that deal with alignedPtr? I think we definitely don't want the users to have to worry about alignment, which is an entirely internal matter. So it makes no sense for them to stay in the user visible header. And yes helper functions like computeStridesFromSizes definitely don't belong in the user visible header.
Perhaps a slightly easier way is that, if we are not entirely sure which functions should be exposed to the end users, we can start by removing those that we know definitely don't belong in the user visible header.

@gongsu832 , that makes sense, so it looks like the majority of the API functions will stay public. If this is the case, then we needn't declare the internal APIs in any header at all. At least for my use case (calling them from within LLVM), header files are not useful. Do you need these declarations in JNI?

@tjingrant No. The JNI wrapper is essentially a "normal user".

Will look at it. Is there anyway that the doxygen files be generated and not included in the GitHub? Like "make doc" would gen them? If its too much work, forget about it.


PaddingFree

Performance testing:

This may be replaced by the official HF solution once its merged https://github.com/huggingface/transformers/pull/31446/


Fix wording in secrets.md

Proposed changes
Updated wording in a sentence because it wasn't written correctly.

Thanks, @ynnadrules :)


not getting inhibited by waybar idle inhibitor

Even waybar idle inhibitor is enabled, hypridle triggers lock screen and system suspend

wasnt that a bug in hyprland 0.40 and fixed in git?

Uhh .... Yeah ur right ....
I forgot that I switched to stable version as u r working on xwayland/clipboard MR


docs:cloudpickle is an interesting extension of the pickle functionality

Description

Quote from their github: "cloudpickle makes it possible to serialize Python constructs not supported by the default pickle module from the Python standard library."
Its already implicity supported by kedro / kedro-plugins, but would be useful to have it in the docs.
Development notes

Changed the documentation
Added cloudpickle to the unit tests
Checklist

[x] Opened this PR as a 'Draft Pull Request' if it is work-in-progress
[x] Updated the documentation to reflect the code changes
[ ] Added a description of this change in the relevant RELEASE.md file
[x] Added tests to cover my changes

Love this! Question for the group, should we think about changing the default pickle engine to something a bit more ML ready?

Cloudpickle is really good, we use it by default in Kedro-AzureML/Sagemaker/VertexAI 💪🏻

Its already implicity supported by kedro / kedro-plugins, but would be useful to have it in the docs.

Would it be possible to make an issue over on the Kedro list please with the docs you think are needed?

Its already implicity supported by kedro / kedro-plugins, but would be useful to have it in the docs.

Would it be possible to make an issue over on the Kedro list please with the docs you think are needed?

Just to clarify: An issue or a pull-request (ie this same pull-request presumably)?

Hi @hfwittmann
Hope this answers your question although I may have misinterpreted it. You wouldn't be able to make docs changes on this PR because the docs are in a different repo, and we don't expect you to do that task too, although of course it's great if can contribute the changes.
Feel free to raise an issue over on https://github.com/kedro-org/kedro/issues to record the fact the work is done and then, if you do want to do that change, feel free to make a separate PR.
Jo

Hi @hfwittmann
Hope this answers your question although I may have misinterpreted it. You wouldn't be able to make docs changes on this PR because the docs are in a different repo, and we don't expect you to do that task too, although of course it's great if can contribute the changes.
Feel free to raise an issue over on https://github.com/kedro-org/kedro/issues to record the fact the work is done and then, if you do want to do that change, feel free to make a separate PR.
Jo

Excellent, I understand now.
Just to be clear, do I need to do this before the pull request is approved?

Just to be clear, do I need to do this before the pull request is approved?

Hi @hfwittmann No, there's no dependency between them. I have re-requested review from @marrrcin as someone well-placed to assess your PR, and all being well, with second approval, we can merge your change.

@merelcht I didn't want to merge this without your signoff (it has 2 approvals but wanted to confirm it's OK to go in). Feel free to merge if you're happy!

@merelcht I didn't want to merge this without your signoff (it has 2 approvals but wanted to confirm it's OK to go in). Feel free to merge if you're happy!

I updated the release notes and now it's all ready to go! 👍


Update testcontainers-scala-core to 0.39.6

Updates com.dimafeng:testcontainers-scala-core from 0.39.5 to 0.39.6.
I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.
If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.
Configure Scala Steward for your repository with a .scala-steward.conf file.
Have a fantastic day writing Scala!

Ignore future updates
Add this to your .scala-steward.conf file to ignore future updates of this dependency:
updates.ignore = [ { groupId = "com.dimafeng", artifactId = "testcontainers-scala-core" } ]

labels: test-library-update, semver-patch

Superseded by #475.


A method of combining two mesh objects

So I was attempting to combine to two mesh objects into one but I was running into issues. The position data was correct, but the normals, tangents, bitangents, and UV's were messed up. The reason I was doing this is to get the same effect as Kirby's Game and Watch pan, which uses vertex order to render the handle last. (Shown in first two images)  I confirmed this after getting the opposite effect by reversing vertex_indices.  (Shown in third image) The ZIP has the pan's vanilla files and my attempt at a combining script (too long to post here).
copy_gamewatch_pan.zip

What files are you running the script with? The reason the model is effected by "vertex order" is because writes to the depth buffer are disabled for most of Game and Watch's meshes by setting o.disable_depth_write = True. The order normally doesn't matter because of depth writes and depth testing.

If you want to reverse the render order of vertices, make sure to reverse the order of the faces and not the entire vertex index list. The winding order is important for face culling or if you want to calculate new normals. If you want to reverse indices [0, 1, 2, 3, 4, 5] you would get [3, 4, 5, 0, 1, 2].

Oh thank you. I had no clue that o.disable_depth_write attribute was a thing. When reversing vertex indices, is it always in groups of three?

Yeah. You can assume each face is 3 consecutive indices for Smash Ultimate.


Web/Safari - History - Images are not displayed after navigating to the conversation while offline

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Version Number: 9.0.57
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5171771&group_by=cases:section_id&group_order=asc&group_id=306202
Issue reported by: Applause - Internal Team
Action Performed:

If you don't have a conversation with images - Navigate to a conversation and send an few images
Navigate to another conversation
Disable the internet connection
Navigate back to the conversation with the image
Verify the images are displayed in the conversation history

Expected Result:
Images that have been previously loaded should display in the conversation history after navigating to the conversation while offline
Actual Result:
Images that have been previously loaded are not displayed in the conversation history after navigating to the conversation while offline
Workaround:
Unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Standalone
[ ] Android: HybridApp
[ ] Android: mWeb Chrome
[ ] iOS: Standalone
[ ] iOS: HybridApp
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/5f98e75e-cf23-4863-9e69-440ec3fcadb8
View all open jobs on GitHub

Tester likely has browser cache disabled, in which case I think this is the expected behavior

@lanitochka17 can you confirm if the browser cache is disabled when you are testing in offline mode?

@lanitochka17 bump!

@johncschuster The browser cache is disabled

Thanks! It sounds like this behavior is expected then. I would suggest two tests, one with the browser cache enabled, and one with the browser cache disabled.
When the browser cache is enabled, I would expect the images to be visible when taking step 4 in your repro steps above, but when the browser cache is disabled, I would expect the images to not be visible when taking step 4 in your repro steps.

@lanitochka17 I think you meant to say that the cache is enabled as the "Disable Caches" checkbox is not checked in the screenshot.
Anyway, it seems that Safari always disables the cache when running in "Private Browsing Mode", even if "Disable Caches" is unchecked. I would suggest updating the test steps to say not to use private mode in Safari.

@lanitochka17 bump!

Asked in Slack to get cleared repro steps

When the cache is disabled, it is reproducible.

Now it's not reproducible with cache enabled
https://github.com/user-attachments/assets/51edeeca-8801-4bf3-a680-f6bbbd8ec5e1

Got it. Thanks! Ok so, this sound like expected behavior.


BUG: Hotbar & other positions messed up with 1.6.6

The position of a bunch of stuff gets messed up with this latest version of Minimal UI. Setting the Hotbar to Manual seems to default 0 to the center of the screen now. Having the Player List window minimized, but then clicking on it which expands it out, seems to shift both the Hotbar and the Scene List at the top of the screen to the side. I'm sure there's probably other positional issues too but I just downgraded to resolve them.

Thanks for letting me know, I'll try to figure out what's wrong. The scene navigation moving to the right I could reproduce, but not the Hotbar moving.

My settings for that were to set the hotbar position to manual, with the position set to 100. In the previous version this would put it very far to the left, almost to the point that it was in contact with the player list. In the latest version 100 puts the leftmost side of it in the center of the screen, or close to it.

Got it, yeah, I didn't see that coming but true a side effect is the change of the meaning of the relative values. The number seems to be way more sensitive now.

Ha, I found the bug. I was adding 400 + 0, which was read as '4000' hence the sensitivity of the hotbar lol. Dang I'm getting rusty. Will fix in a moment

Hi @GambetTV - mind giving me a second chance and test?

It mostly seems good, but when the player list goes from minimized to expanded (as in, when you can only see the dots to you can see the player's names) the hotbar shifts to the right, which it didn't do before.

Cool, I couldn't reproduce that moving hotbar but will see.

You sure you tried Minimal UI 1.6.7 @GambetTV ? I can't reproduce the scene navigation moving or the hotbar shifting upon expanding the player's list anymore.

Yeah, it's 1.6.7, but it seems like it might be a bug caused by your module interacting with the module "Monk's Hotbar Expansion." However, it's gotta be your module causing the issue, because it works fine in 1.6.5, and MHE hasn't been updated in over a year.

Ahh gotcha, yes I might have broken specific module compatibility with Monk's. Will check quickly.

Alright, I reviewed this. @GambetTV - this really isn't my fault. This used to work because the player's list was hardcoded, just like Monk's. Now Minimal UI behaves like Foundry VTT does, which is cleaner and more compatible.
The fix needs to be done on Monk's side, to not have a fixed position on his side. But the current approach in Minimal UI is the best for the bigger picture.

Okay, no worries. I'll put in a ticket for Monk's, although I'm not sure I'll hold my breath it'll ever get resolved, as the module hasn't been updated in over a year. In the meantime I'll probably have to downgrade to 1.6.5 just to make sure everything stays working. Thanks for looking into it though!

Actually, now that I think about it this might be even better. I suppose I can just remove the buffer, which will save me more space here since I mostly have the player list minimized anyway. TBH I didn't really think about the utility of it, I just saw that it didn't work how it used to and assumed it was a bug.

Thanks - in any case, I just found a way to make an exception and make them work together for a longer while. Will release soon, but yeah it is mostly an exception specifically for Monk's hotbar. Maybe there are other users who want to use both

Really fixed now, compatibility with Monk's Hotbar for a longer while :)


Integration Test: Clients can negotiate TLS 1.2 with TLS 1.3 configured S2N server

Problem:
A s2n server can be configure with a list of cipher suites that is a combination of TLS 1.3 and TLS 1.2. Ensure that a client can negotiate and establish a TLS 1.2 connection correctly if the server picks a TLS 1.2 cipher suite.
Proposed Solution:

Related: https://github.com/awslabs/s2n/issues/1726


HttpRequest composing Context

Recently we were having discussions around updating HttpRequest to compose util.Context. The reason was - we came across cases where we want to communicate request-specific flags to the HttpClient. Still, we don't want to expose such flags to the user as APIs; it's more of internal details. one example case is telling HttpClient to eagerly-read a specific request's response, a flag we want to set from RestProxy when it knows that eager read is more optimal.
We discussed about HttpRequest composing util.Context but that makes HttpPipelineCallContext point-less class. Given this involves breaking changes, for now we decided to add a new API on HttpClient, taking Context in addition to HttpRequest pr-ref. Opening this issue so that the redesign of the pipeline and HttpClient APIs for azure-core vNext can include this requirement.
\cc @alzimmermsft @srnagar

I like the idea of composing Context in HttpRequest but we might still need HttpPipelineCallContext to configure pipeline-specific settings  that are not HttpRequest related.

I like the idea of composing Context in HttpRequest but we might still need HttpPipelineCallContext to configure pipeline-specific settings  that are not HttpRequest related.

Thanks, Srikanta. Updating this thread based on our further discussion.
We'll clearly separate the contextual data for "Pipeline-Run" and "HttpRequest".
For pipeline-run we can use util.Context which is immutable and thread-safe.  It's the Context that the user provides to the API call. A pipeline policy can further pass (after modification optionally) it to other APIs requiring Context. This means the spanning of such API calls plays nicely with the tree structure of Context. Due to the immutable nature of Context, all modifications later in the call-stack is not visible to parent, hence not visible to the user; this is exactly what the user expects by using Context.
For HttpRequest, it's the contextual data very local to the request instance; this means we don't really need the complex data structure that Context offer; we can use a simple map or a properties bag.

Thanks, Srikanta. Updating this thread based on our further discussion.
We'll clearly separate the contextual data for "Pipeline-Run" and "HttpRequest".
For pipeline-run we can use util.Context which is immutable and thread-safe.  It's the Context that the user provides to the API call. A pipeline policy can further pass (after modification optionally) it to other APIs requiring Context. This means the spanning of such API calls plays nicely with the tree structure of Context. Due to the immutable nature of Context, all modifications later in the call-stack is not visible to parent, hence not visible to the user; this is exactly what the user expects by using Context.
For HttpRequest, it's the contextual data very local to the request instance; this means we don't really need the complex data structure that Context offer; we can use a simple map or a properties bag.


fix for detectors with sigma aggregation rules

Description
fix for detectors with sigma aggregation rules
Related Issues
Resolves #[Issue number to be closed when this PR is merged]

Check List

[X] New functionality includes testing.
[ ] New functionality has been documented.
[ ] API changes companion pull request created.
[X] Commits are signed per the DCO using --signoff.
[X] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

when is template applied? not at creation time? why is there sleep 90 s in test?

Yes. Template is applied at creation time with the Create Mapping api call.


Project page count issue bcoz of Search By Staff Role filter

Project page count issue because of Search By Staff Role filter
Pending count is 6,

But when i click on lead, project is not appeared. Because staff role filter is not change to executive.

Problem is :
by Default Project and Status both load as Executive
and Role filter is set to Project Manager
so second time click on any filter gives project as Manager,
that makes project count change.

Roles are setting in Manager, Project Manager then Executive Order,
That is messing things as it takes time to load page.
so now setting manager then directly executive , not setting Project Manager role as it show lower number of project.
@vinpahilwani required that executive should be default role not Project Manager, because it show lower number of projects.
this things were working perfectly before, but due to home page click required new roles and preloading Role filter made this error


button grayed out

Hi Marco, great idea, I am used to to this same technique but manually, finding out the SSAS Tabular port.
The button is grayed out.
I installed using the MSI you posted in the previous issue.
Mariano

Go to Preview features in Power BI, check "Store datasets using enhanced metadata format", and restart Power BI, that must fix the problem, showing external tools enabled.
Let me know if this fixes the problem.

Hi Marco, I applied the changes you mentioned and it is working now.
When Power BI Desktop is closed, and then opened again, the SSAS Tabular port will change. To refresh the objects created in a previous session, is changing the port in the connection properties the best approach to reconnect ?

Currently yes - in future versions of Analyze in Excel we should implement techniques to refresh the connection automatically.


Add note about kotlin upgrade to upgrade guide

@bot-gradle test and merge

OK, I've already triggered a build for you.


Anime Download Issue

Steps to reproduce

Open anime from any extensions.
Click on download button wait 2 sec.
It will stop downloading automatically and won't give any error message.

Expected behavior
Episode should be downloaded
Actual behavior

Crash logs
No response
Aniyomi version
App version: <IP_ADDRESS>
Android version
Android 10
Device
Poco X2
Other details
No response
Acknowledgements

[X] I have searched the existing issues and this is a new ticket, NOT a duplicate or related to another open issue.
[X] I have written a short but informative title.
[X] If this is an issue with an extension, I should be opening an issue in the extensions repository.
[X] I have tried the troubleshooting guide.
[X] I have updated the app to version <IP_ADDRESS>.
[X] I have updated all installed extensions.
[X] I will fill out all of the requested information in this form.

I am getting same issue with other extensions amd zoro.to extension is not working at all...

I am getting same issue with other extensions amd zoro.to extension is not working at all...

And Im getting in on 9anime and geno

Try switching the quality of the video....
Go to Browse> Anime extensions> 'Extension Settings' Press the gear icon and set the video quality....
Some anime providers don't have 1080p quality...
I personally prefer 480p.. since it uses less data and has enough clarity..

im pretty sure i found the cause of this bug.
ffmpeg doesnt work well with commas in the http header options.
i will fix this in the next update.
maybe the extensions will also be updated to fix this.

fixed by 98b7e1dbebb9460a7a036f3c738772be9121b6fa


vttablet: Follow up fixes for new ParseTabletTypes method.

Alain, you merged https://github.com/youtube/vitess/pull/2871/files a bit too fast :)
Here are the changes I would have asked for during a code review.

LGTM


pull request for e2e test at 11/10/2016 19:56:09

:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

optest/docs/updated_at.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.


client.keyStats - 404

Describe the bug
Maybe this isn't a bug and I'm consuming the library poorly. The keyStats method on Client is kicking back a 404.
To Reproduce
Steps to reproduce the behavior:
const client = new Client({ version: "sandbox", api_token: process.env.REACT_APP_API_KEY }); client.keyStats('APPL', 'companyName', 'symbol,week52high', 'json').then(console.log)
Expected behavior
There should be a json payload
Desktop (please complete the following information):

Big Sur
chrome
Version 90.0.4430.93

Additional context
Add any other context about the problem here.

 I was using an invalid symbol


Test MockSqlQueryT

Part 2/N for #2

Codecov Report

Merging #13 (d1cb747) into master (e8b1c1d) will increase coverage by 0.75%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   40.90%   41.66%   +0.75%
==========================================
  Files           5        5
  Lines         264      264
==========================================
+ Hits          108      110       +2
+ Misses        156      154       -2

Impacted Files
Coverage Δ

src/Database/Persist/Monad/TestUtils.hs
100.00% <0.00%> (+15.38%)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8b1c1d...d1cb747. Read the comment docs.


RFR: Refactor heckell differ

Refactored our implementation of the Heckel Differ to be (hopefully more maintainable).
For the time being I am keeping the original version of the differ around so we can make sure the refactors are sane.
Also broke up the tests into smaller, files to more easily navigate them.
Removed some legacy and unused files as well.

@jelder I decided to keep the messy but stable differ around to make sure the new one produces the same results. Planning to update versioned_record and www to run both differs for a time and honeybadger any differences, while still using the stable but messy one.

I like it. Think it will have any noticeable performance impact?
Also if you find that there are some areas of intractable complexity, you can silence Rubocop:
https://github.com/bbatsov/rubocop#disabling-cops-within-source-code

@jelder since we are using the original differ for production tasks for now, we could put the comparison into sidekiq and not worry about it.

Could be tricky with Dyph3 not currently being aware of Sidekiq.


Implement  last/first to aggregateBy

I'm submitting a ...

[ ] Bug report
[x] Feature requestdd
[ ] Question / Support request

What did you do?
Able to get last or first value in a interval using aggregateBy or another function, if need.
What was the expected result?
The value of point will be the last or first value to a given interval.
For example, aggregateBy(1d, last) must return last value in the resulting serie of interval "1d". Just like other functions do (avg, min, max), just allow get first or last. (if possible, we can specifu a Nth position. But first or last is good for while!)
What happened instead?
Not implemented yet. Just avg, max and min is supported.

I have a different use case for "last" aggregation.
I want to expose the number of servers for a particular hostgroup.
Using "last" + "count" will be easy to get that result.
Another use case si to obtain the total memory sum for all servers. "last" + "sum" will be handy also.

Any update on this issue? I have the same problem.
I have several items loaded from LLD which are booleans ( 1 or 0).
I need to show a count of how many are 1 and how many are 0.
Other ideas are also welcome ;-)
Thanks!


Added password hashing and compare as a part of user schema

Added password hashing and compare logic in user schema

Please review, @Anmolbansal1 , @ivantha

@Pradip-001 Please rebase this 🙂

@ivantha , please review , i have changed according to updated master also changed password incorrect status code

@ivantha , @Anmolbansal1 please review the changes


feature: 同步 CMDB 主机 cpu 架构及操作系统信息

Codecov Report

Merging #1550 (67593b2) into v2.3.2-dev (4e0002b) will decrease coverage by 0.11%.
The diff coverage is 62.33%.

@@              Coverage Diff               @@
##           v2.3.2-dev    #1550      +/-   ##
==============================================
- Coverage       73.40%   73.29%   -0.11%
==============================================
  Files             386      387       +1
  Lines           25974    26115     +141
==============================================
+ Hits            19067    19142      +75
- Misses           6907     6973      +66

Impacted Files
Coverage Δ

apps/node_man/periodic_tasks/sync_cmdb_host.py
62.62% <57.50%> (-2.41%)
:arrow_down:

apps/node_man/tools/host_v2.py
82.29% <64.70%> (-6.74%)
:arrow_down:

apps/node_man/constants.py
96.36% <100.00%> (+0.02%)
:arrow_up:

apps/node_man/models.py
76.58% <100.00%> (+0.03%)
:arrow_up:

... and 23 files with indirect coverage changes


Roll Flutter Engine from cd0a6ef93263 to 7dda64947930 (3 revisions)

https://github.com/flutter/engine/compare/cd0a6ef93263...7dda64947930
2023-03-23<EMAIL_ADDRESS>[impeller] dont apply opacity peephole on image/color filters (flutter/engine#40551)
2023-03-23<EMAIL_ADDRESS>[impeller] always copy bitmaps when given unencoded data (flutter/engine#40543)
2023-03-22<EMAIL_ADDRESS>Run windows_arm build only on main branch. (flutter/engine#40546)
If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry<EMAIL_ADDRESS>on the revert to ensure that a human
is aware of the problem.
To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose
To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug
Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Commit queue failed; closing this roll.


multiChart throws 'TypeError: Cannot read property 'y' of undefined' when we have more than one 'area' chart in yAxis1

multiChart throws TypeError: Cannot read property 'y' of undefined when we have more than one 'area' chart in yAxis1
Plunker: http://plnkr.co/edit/m9mIM9yiRHBauWzKRA5k?p=preview

It's expecting the same number of data points across each series - just add some dummy points to extend the length to be equivalent and it will work.


fix: PDF Ingestion bug when Grobid is unable to parse the reference PDF

https://github.com/refstudio/refstudio/issues/79

This fixes two underlying bugs:

When Grobid is unable to parse a PDF, it prints an error message to stdout, rather than raising an exception or returning an http error status code. Printing this error message to stdout breaks sidecar communication since we rely on stdout for data passing.
If a PDF could not be parse, it is not included in the References response from the sidecar. This adds Reference objects to the response even if we could not parse the PDF.

I'm still seeing brittle behavior after this fix...


typo fix in README.md

Summary by CodeRabbit

Documentation

Corrected the terminology from "tatics" to "tactics" in the README.md, enhancing clarity on the kernel's capabilities.

Thanks for spotting this!


DebugSettings.EnableFrameRateCounter = true in Desktop causes an AccessViolation

Describe the bug
Create a new WinUI Desktop application and set the DebugSettings.EnableFrameRateCounter = true in the application constructor or the OnLaunched and an access violation exception will occur when the application is run.
Exception thrown at 0x7A0D38FC (Microsoft.UI.Xaml.dll) in WinUI_InDesktop.exe: 0xC0000005: Access violation reading location 0x00000000.

Steps to reproduce the bug

In the attached solution, run the WinUiDesktop (Package) application.  Note that you will be able to safely step over enabling the frame rate counter but the application will crash shortly after.
For comparision, using the same solution, run the BlankAppUWP (Package).  Note that the application will not crash and it will show the framerate counter as expected.

Expected behavior
If by chance the DebugSettings.EnableFrameRateCounter setting cannot be used with Desktop applications, then a non-fatal exception message should be thrown.  If this feature is supported in Desktop applications then it has a bug.
Screenshots

Version Info
.Net5.0 Preview 6
NuGet package version:
    <PackageReference Include="Microsoft.WinUI" Version="3.0.0-preview1.200515.3" />
Note that I have been making WinUI work with Preview 6 by adding this additional direct package reference, a trick I learned from Scott Jones.  Although I have been able to accomplish quite a bit of Window related work without any crashes, it was worth mentioning.
<PackageReference Include="Microsoft.Windows.CsWinRT" Version="0.1.0-prerelease.200623.5" />

Windows 10 version
Saw the problem?

Insider Build (xxxxx)

May 2020 Update (19041)
Yes

November 2019 Update (18363)

May 2019 Update (18362)

October 2018 Update (17763)

April 2018 Update (17134)

Fall Creators Update (16299)

Creators Update (15063)

Device form factor
Saw the problem?

Desktop
Yes

Xbox

Surface Hub

IoT

Additional context
I have attached a solution for easy reproduction of the problem.
WinUIDesktop.zip

Also happens with Preview2

@RBrid Fyi

Tracked by internal task 27649649 (and originally bug 27512832).

This is still happening as of today.

Also today

Fixed in WinAppSDK 1.4-experimental1 on 05/23/23; closing bug.

Fixed for the next 1.5 release.

Automation error. This is fixed in the 1.5 release.


Keep Stash History

It would be nice to be able to keep stash history so that when we view a node we can see what has happened to it before.

This is a planned feature. It will probably be rolled into a general events tracking feature that I haven't fleshed out yet.


React Redux gives SyntheticEvent instead of boolean randomly in mapMergeToProps

In a react application, I am encountering issue with connect 3rd parameter which is mergedProps.
Expected
I expect to always receive a boolean
Result
I often get a boolean, but I randomly get a SyntheticEvent
Code
import React from 'react';
import PropTypes from 'prop-types';
import Switch from 'expo-custom-switch';
import color from 'color';
import { withTheme } from 'react-native-paper';
import { connect } from 'react-redux';
import { StyleSheet, View } from 'react-native';
import { compose } from 'redux';
import { createStructuredSelector } from 'reselect';
import { onHasDrawerChange } from '../../core/reducers/app/layoutReducer/actions';
import { changeColorScheme } from '../../core/reducers/app/preferencesReducer/actions';
import { proChange } from '../../store/icimatinLayout/actions';
import { selectPro } from '../../store/icimatinLayout/selectors';
import { selectIsLoggedIn } from '../../store/session/selectors';

const styles = StyleSheet.create({
  container: {
    marginTop: 8,
    marginRight: 16,
    marginBottom: 8,
    marginLeft: 8,
  },
});

function SwitchPro({
  theme: { colors },
  pro,
  onChange,
  onPress,
  isLoggedIn,
  ...rest
}) {
  return (
    <View style={styles.container}>
      <Switch
        value={!pro}
        onChange={(newValue) => {
          onChange(!newValue);
          if (onPress) {
            onPress(!newValue);
          }
        }}
        leftColor={color(colors.pro).lighten(0.2).toString()}
        rightColor={color(colors.primary).lighten(0.3).toString()}
        iconLeft={{
          name: 'account-supervisor-circle',
          size: 30,
          color: '#fff',
          style: {
            height: 31,
            width: 30,
          },
        }}
        iconRight={{
          name: 'account',
          color: color(colors.white).lighten(0.2).toString(),
          style: {
            height: 22,
            width: 22,
          },
        }}
        {...rest}
      />
    </View>
  );
}

SwitchPro.propTypes = {
  /** @ignore */
  theme: PropTypes.object.isRequired,
  /** @ignore */
  onChange: PropTypes.func.isRequired,
  /** @ignore */
  pro: PropTypes.bool.isRequired,
  /** @ignore */
  isLoggedIn: PropTypes.bool.isRequired,
  /** @ignore */
  onPress: PropTypes.func,
};

SwitchPro.defaultProps = {
  onPress: undefined,
};

const mapStateToProps = createStructuredSelector({
  pro: selectPro,
  isLoggedIn: selectIsLoggedIn,
});

export const mapMergeToProps = (stateProps, dispatchProps, ownProps) => ({
  ...ownProps,
  ...stateProps,
  ...dispatchProps,
  onChange: (value, ...rest) => {
    if (typeof value !== 'boolean') {
      // TODO: find why value is not boolean
      console.log('switchpro', value, ...rest);
    }
    dispatchProps.dispatch(changeColorScheme(value ? 'dark' : 'light'));
    dispatchProps.dispatch(proChange(value));
    dispatchProps.dispatch(onHasDrawerChange(stateProps.isLoggedIn));
  },
});

export default compose(
  withTheme,
  connect(mapStateToProps, undefined, mapMergeToProps),
)(SwitchPro);

Logs
When I have the wrong value, this is what I log instead of the boolean:
switchpro SyntheticEvent {
  "_dispatchInstances": Array [
    FiberNode {
      "tag": 5,
      "key": null,
      "type": "AndroidTextInput",
    },
    FiberNode {
      "tag": 5,
      "key": null,
      "type": "RCTScrollView",
    },
  ],
  "_dispatchListeners": Array [
    [Function bound _onChange],
    [Function onChange],
  ],
  "_targetInst": FiberNode {
    "tag": 5,
    "key": null,
    "type": "AndroidTextInput",
  },
  "bubbles": undefined,
  "cancelable": undefined,
  "currentTarget": 833,
  "defaultPrevented": undefined,
  "dispatchConfig": Object {
    "phasedRegistrationNames": Object {
      "bubbled": "onChange",
      "captured": "onChangeCapture",
    },
  },
  "eventPhase": undefined,
  "isDefaultPrevented": [Function functionThatReturnsFalse],
  "isPropagationStopped": [Function functionThatReturnsFalse],
  "isTrusted": undefined,
  "nativeEvent": Object {
    "eventCount": 4,
    "target": 749,
    "text": "test",
  },
  "target": 749,
  "timeStamp":<PHONE_NUMBER>681,
  "type": undefined,
}

Versions

react ~16.13.0
react-redux ^7.2.0
redux ^4.0.5"
expo SDK 36

I am not able to reproduce this easily, any idea what is happening and how I can prevent SyntheticEvent from being injected here ?

The issue tracker here on GitHub is reserved for bug reports and feature requests. For usage questions (which is what I believe this is), please use Stack Overflow or Reactiflux where there are a lot more people ready to help you out. Thanks!

@markerikson why do you say it is not a bug? It's obviously a bug.

We don't have any control over what parameters are being passed into your callback.  That's specific to your app.

The thing is, I never had such error, and I don't often use mapMergeToProps in my app, it only happen at this point of my app.
But your explanation makes sens. I don't have the control my self on it. Well, that means the error comes from react-native, I fixed this by forcing the boolean like this:
    if (typeof value !== 'boolean') {
      // TODO: find why value is not boolean
      console.log('switchpro', value, ...rest);
      value = !!value;
    }


NuGet

Hey,
what about compiling the library and publishing on NuGet?

Hi Matteo! :)
Well, it's not in a state yet where it can be published. There a lot of bugs and incorrect implementations.
But I will try to get a first version ready within the next weeks - took me way too long now.


promise resolves with result.insertId = 0

I call an insert query and expect an integer > 0.
Instead I get insertId = 0. After querying the db manually, the id is correct?
What possible ways are there to debug this?
I have console logged the returnted result,
checked the db schema and logs, but everything is as expected..

.

Not sure. There are no promises in this module.

Closing due to no response.

@Sosomqk did you ever resolve your issue? Facing the same now.

@chrise86 I do not. Can't quite remember how and if I fixed it, but try to dump the whole results object and check the id. Also make sure you are always awaiting the returned promise wherever you call the function


fix(menu): prevent extra '/' in menu link for empty path string

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
Menu item link contains extra '/' for empty string path component. Please see https://github.com/akveo/ng2-admin/issues/364.

What is the new behavior (if this is a feature change)?
Prevent extra '/'.

Other information:
n/a

Thanks @AlbertXingZhang!

No problem. Thanks for merging!


Make sure that ? is an alias for help

in docs phr ? help is featured, while only phr help help works
previously it worked, so functionality needs to be restored

interestingly enough, ? work with bash shell and fails only with zsh


BUG PMNC errors out pulling image

PMNC after a while will unprovoked error out with the following logs
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling... ERROR: (utun4) 2024/04/22 15:36:18 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF DEBUG: (utun4) 2024/04/22 15:36:18 peer(HFvW…nIHE) - Handshake did not complete after 5 seconds, retrying (try 3) DEBUG: (utun4) 2024/04/22 15:36:18 peer(HFvW…nIHE) - Sending handshake initiation DEBUG: (utun4) 2024/04/22 15:36:23 peer(HFvW…nIHE) - Handshake did not complete after 5 seconds, retrying (try 2) DEBUG: (utun4) 2024/04/22 15:36:23 peer(HFvW…nIHE) - Sending handshake initiation Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...

I don't know what the cause of this is, but I had to force kill the process and start it up again to get passed this error

I have seen similar before:
tail -f /opt/homebrew/var/log/podman-mac-net-connect/std_out.log
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:58:39 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:58:45 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:58:51 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:58:57 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:59:04 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:59:10 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF
Image (quay.io/jmadigan/podman-mac-net-connect:0.0.8) doesn't exist locally. Pulling...
ERROR: (utun8) 2024/06/17 14:59:16 Failed to setup VM: failed to pull setup image: Post "http://d/v4.7.0/libpod/images/pull?reference=quay.io%2Fjmadigan%2Fpodman-mac-net-connect%3A0.0.8": EOF

Haven't gotten to the bottom of it yet, but will dig further when it re-occurs. Restarting the service (brew service restart podman-mac-net-connect) resolves for time being.

Think this has been fixed recently.

Same issue here , not resolved yet.
Need to restart brew service to resolve. I don't know if it is linked to another issue when sometime host lost connection:
Request timeout for icmp_seq 742
Request timeout for icmp_seq 743
Request timeout for icmp_seq 744
64 bytes from <IP_ADDRESS>: icmp_seq=745 ttl=63 time=14.238 ms
64 bytes from <IP_ADDRESS>: icmp_seq=746 ttl=63 time=0.856 ms
64 bytes from <IP_ADDRESS>: icmp_seq=747 ttl=63 time=1.697 ms
64 bytes from <IP_ADDRESS>: icmp_seq=748 ttl=63 time=1.437 ms
64 bytes from <IP_ADDRESS>: icmp_seq=749 ttl=63 time=1.606 ms
64 bytes from <IP_ADDRESS>: icmp_seq=750 ttl=63 time=7.313 ms
64 bytes from <IP_ADDRESS>: icmp_seq=751 ttl=63 time=2.007 ms
64 bytes from <IP_ADDRESS>: icmp_seq=752 ttl=63 time=3.217 ms
64 bytes from <IP_ADDRESS>: icmp_seq=753 ttl=63 time=2.734 ms
64 bytes from <IP_ADDRESS>: icmp_seq=754 ttl=63 time=1.349 ms
Request timeout for icmp_seq 755
Request timeout for icmp_seq 756
Request timeout for icmp_seq 757
Request timeout for icmp_seq 758
Request timeout for icmp_seq 759
Request timeout for icmp_seq 760
Request timeout for icmp_seq 761
Request timeout for icmp_seq 762
Request timeout for icmp_seq 763
Request timeout for icmp_seq 764
64 bytes from <IP_ADDRESS>: icmp_seq=765 ttl=63 time=1.730 ms
64 bytes from <IP_ADDRESS>: icmp_seq=766 ttl=63 time=2.324 ms
64 bytes from <IP_ADDRESS>: icmp_seq=767 ttl=63 time=1.637 ms
64 bytes from <IP_ADDRESS>: icmp_seq=768 ttl=63 time=1.128 ms
64 bytes from <IP_ADDRESS>: icmp_seq=769 ttl=63 time=1.564 ms
64 bytes from <IP_ADDRESS>: icmp_seq=770 ttl=63 time=4.845 ms
64 bytes from <IP_ADDRESS>: icmp_seq=771 ttl=63 time=2.797 ms
64 bytes from <IP_ADDRESS>: icmp_seq=772 ttl=63 time=1.610 ms
64 bytes from <IP_ADDRESS>: icmp_seq=773 ttl=63 time=2.597 ms
64 bytes from <IP_ADDRESS>: icmp_seq=774 ttl=63 time=0.698 ms
64 bytes from <IP_ADDRESS>: icmp_seq=775 ttl=63 time=1.050 ms
Request timeout for icmp_seq 776
Request timeout for icmp_seq 777
Request timeout for icmp_seq 778


samples: net: google_iot_mqtt: log actual error

currently 0 logged always

changed the title ~samples: log actual error in google_iot_mqtt~ samples: net: google_iot_mqtt: log actual error 8 minutes ago

You need to change the commit for re-submit (force push) the PR, it is not enough to edit the PR title.

Sure - done


[WIP] Merge spans longer than 3 reserved sections

Identify these spans by the starting section number. Update the section
title to indicate the sections in the span.

Question: Is there ever a span of sections that are not RESERVED?

This looks good!
Looks like test failures are integration tests for FEC and EPA, which probably both have runs of > 3 reserved sections. Would you mind verifying the data on FEC and I'll take a look at EPA?

Is there ever a span of sections that are not RESERVED?

Good question -- we haven't seen any that weren't reserved across the four eRegs agencies, so I think that's a safe bet now.

Should the span indicator be explicit rather than implicit in the title?

I don't follow; would you mind giving an example?

I'll verify the integration test failures for FEC.

Should the span indicator be explicit rather than implicit in the title?
In https://github.com/eregs/regulations-parser/blob/767aa18e7f5d8297c6ddc268ab079d8f37dcc09a/regparser/tree/xml_parser/reg_text.py#L256, we already know that we are dealing with sections and/or section spans. We don't indicate this information explicitly in the persisted output. It is implicit in the label and the title. In https://github.com/eregs/regulations-site/blob/23fd2ab001782a9654e26496aa96870d16b71614/regulations/generator/title_parsing.py#L40, we extract this information again using similar logic.

@vrajmohan gotcha. I think they're working slightly different angles. The build_from_section logic is meant to only concern itself with the creation of Node objects, so it cares about spans of sections only to the extent that it needs to create multiple Nodes or not. We're doing a bit of reformatting there, but I think that's more out of convenience than cleanliness.
I don't think that section-spanning data is very important to Nodes; but it is important to the table-of-contents layer. I'd argue that it should be explicit there. Unfortunately the relevant logic is actually within -site :(. We run into a similar problem in the N&C code -- it'd be ideal if the parser was doing the parsing rather than the UI.

I can confirm that the only changes to EPA are expected -- several runs of sections are merged into a single "reserved" section.

Likewise, I can confirm that the only changes to FEC are expected -- several runs of sections are merged into a single "reserved" section.

Looks good, test failures make sense, and @vrajmohan assures this is no longer a WIP


GlobalTableCheckerRole Policy Document causing cfn-template to fail

Tried the One-Click deployment on the AWS-Solutions site using the default region (us-east-1) and I'm receiving the following error:

The policy failed legacy parsing (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx; Proxy: null)

The following resource(s) failed to create: [GlobalTableCheckerRole]. Rollback requested by user.

Hi @gmedard -
Thanks for raising this. I was able to reproduce this by using an invalid value as the Secondary Region when deploying the stack. For instance, I used us-west--2. Can you please try again and see if maybe there was a typo you used when setting that parameter?
We'll also look into tightening the regex that validates that parameter.

Thanks @ericquinones  - There was an extra character being copied when I was pasting.  That fixed that issue.


[Bug]: incomplete DBMS output displayed on the front-end

ODC version
4.2.2
OB version
ob-oracle
What happened?
Incomplete DBMS output displayed on the front-end. It seems that only some blank spaces were displayed.

What did you expect to happen?
All spaces should be displayed.
How can we reproduce it (as minimally and precisely as possible)?
begin
  dbms_output.put_line('dfd' || ' asd   sdf  ' || 'bb');
end;
/

Anything else we need to know?
No response
Cloud
No response

pass


Golang monkey patching library utilized against terms of license

The Golang monkey patching library, https://github.com/bouk/monkey, is being utilized directly against its license:

Copyright Bouke van der Bijl
I do not give anyone permissions to use this tool for any purpose. Don't use it.
I’m not interested in changing this license. Please don’t ask.

It is also archived and not maintained.

Thanks for reporting this issue.
These library codes are just used for unit testing, which is not included in the release.
But we should find an alternative or just write our own monkey patching library.
P.S. there is a discussion you can refer to https://news.ycombinator.com/item?id=28263721

close this issue temporarily as this is a known issue, and we don't have a solution at this moment. I will keep my eyes open if there is an alternative solution.


Fixed the potential error reported by Prefast code analysis

To fix this warning from Prefast:
pdo_sqlsrv\pdo_dbh.cpp(1303) : warning C6385: Reading invalid data from 'last_insert_id_query':  the readable size is '184' bytes, but '2147483647' bytes may be read.

This change is

Isn't it possible that any call to strlen_s would produce this error, since we don't specify the length argument?

@david-puglielli -- in this particular case strnlen_s() was called to determine how many characters are in the input string to be converted. Yet, it should never reach INT_MAX<PHONE_NUMBER>), the limit that we use for strnlen_s() , because last_insert_id_query is either LAST_INSERT_ID_QUERY or SEQUENCE_CURRENT_VALUE_QUERY with the sequence name. However, the code change in this PR is required to get rid of this warning.

Coverage increased (+0.003%) to 74.7% when pulling 10ff344fca84f1393ba809c604395b6ad3006fbd on yitam:fillLastID into<PHONE_NUMBER>503d86b4e90684c93f09a57a0389ea on Microsoft:dev.